[Search v2.3] - Educational tooltip should not show over a dialog

[lakchote]

The educational tooltip should not show over a dialog.

(note: the educational tooltip shows when you create a saved search for the first time)

Steps:

1. Clicked on an expense on the search result
2. Clicked on the receipt thumbnail
3. The tooltip is still showing above the dialog

Issue Owner

Current Issue Owner: @dukenv0307

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 (External)

[abzokhattab]

Should it be hidden permanently, or should it be hidden and then shown again when the receipt dialog is closed?

[lakchote]

Should it be hidden permanently, or should it be hidden and then shown again when the receipt dialog is closed?

If it has shown already, it should not show again when the receipt dialog is closed.

[daledah]

Edited by proposal-police: This proposal was edited at 2023-10-16T07:28:00Z.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Educational tooltip should not show over a dialog

What is the root cause of that problem?

We don't have the logic to check if the modal will be opened in

App/src/components/MenuItem.tsx

Line 555 in 4c9a3fb

shouldRender={shouldRenderTooltip}

when the tooltip is about to open, user quickly open attachment modal, the modal will be shown along with tooltip

What changes do you think we should make in order to solve the problem?

1. Should't show tooltip if the modal is about to open

App/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx

Line 42 in 52c0ccb

useEffect(() => {

add logic to clean up settimeout if should render change from true to false

    const [modal] = useOnyx(ONYXKEYS.MODAL);

    const shouldShow = !modal?.willAlertModalBecomeVisible && !modal?.isVisible
    const didShow = useRef(false);

...
    useEffect(() => {
        if (!shouldRender || !shouldMeasure || !shouldShow || didShow.current) {
            return;
        }
        // When tooltip is used inside an animated view (e.g. popover), we need to wait for the animation to finish before measuring content.
        setTimeout(() => {
            didShow.current = true;
            show.current?.();
        }, 500);
    }, [shouldMeasure, shouldRender, shouldShow]);

2. In case the tooltip will close in 5s, and the modal is open, we should close it

    useEffect(() => {
        if (!hideTooltipRef.current || !shouldAutoDismiss) {
            return;
        }

        // If the modal is open, hide the tooltip immediately and clear the timeout
        if (!shouldShow) {
            hideTooltipRef.current();
            return;
        }

        // Automatically hide tooltip after 5 seconds if shouldAutoDismiss is true
        const timerID = setTimeout(hideTooltipRef.current, 5000);
        return () => {
            clearTimeout(timerID);
        };
    }, [shouldAutoDismiss, shouldShow]);

With this approach, we can make sure the tooltip can't be open when the modal (attachment modal or right modal) is about to show. If users just switch between tabs (there's no modal), we should preserve the tooltip

What alternative solutions did you explore? (Optional)

[abzokhattab]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Educational tooltip shows over a dialog

What is the root cause of that problem?

the educational tooltip hides after 5 seconds regardless of whether a modal is opened or not

App/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx

Lines 30 to 40 in 16fa4b4

	// Automatically hide tooltip after 5 seconds
	useEffect(() => {
	if (!hideTooltipRef.current || !shouldAutoDismiss) {
	return;
	}
	const timerID = setTimeout(hideTooltipRef.current, 5000);
	return () => {
	clearTimeout(timerID);
	};
	}, [shouldAutoDismiss]);

What changes do you think we should make in order to solve the problem?

we should refactor the prev useEffect so that i becomes hidden once a modal is opened:

    const [modal] = useOnyx(ONYXKEYS.MODAL);
    const timerIDRef = useRef<NodeJS.Timeout | null>(null);
    // Hide the tooltip after 5 seconds or immediately if the modal is open
    useEffect(() => {
        if (!hideTooltipRef.current || !shouldAutoDismiss) {
            return;
        }

        // Function to clear the timeout
        const clearTimer = () => {
            if (!timerIDRef.current) {
                return;
            }
            clearTimeout(timerIDRef.current);
            timerIDRef.current = null;
        };

        // If the modal is open, hide the tooltip immediately and clear the timeout
        if (modal?.willAlertModalBecomeVisible) {
            clearTimer(); // Clear the timeout if the modal opens
            hideTooltipRef.current();
            return;
        }

        // Automatically hide tooltip after 5 seconds if shouldAutoDismiss is true
        if (shouldAutoDismiss) {
            timerIDRef.current = setTimeout(hideTooltipRef.current, 5000);
        }
        return () => {
            clearTimer();
        };
    }, [shouldAutoDismiss, modal?.willAlertModalBecomeVisible]);

POC

Screen.Recording.2024-09-20.at.1.05.38.PM.mov

What alternative solutions did you explore? (Optional)

optionally: if we want to make this only applicable on certain usecases we can add a new prop to the BaseEducationalTooltip (shouldDismissOnModalOpen) and call it when needed

[ishpaul777]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Educational tooltip is shown over the all of the content when the search page is in the background

What is the root cause of that problem?

We dont have logic to hide the tooltip when user is navigated from the screen we only have logic to autohide when tooltip after 5s

App/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx

Lines 30 to 40 in 16fa4b4

	// Automatically hide tooltip after 5 seconds
	useEffect(() => {
	if (!hideTooltipRef.current || !shouldAutoDismiss) {
	return;
	}
	const timerID = setTimeout(hideTooltipRef.current, 5000);
	return () => {
	clearTimeout(timerID);
	};
	}, [shouldAutoDismiss]);

What changes do you think we should make in order to solve the problem?

we can setup a navigation listner for navigation state change so as soon as user takes a navigation action tooltip hides, for safety we'll add this change specific when we need with shouldHideOnNavigation prop.

// src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx
// Automatically hide tooltip after 5 seconds
  useEffect(() => {
      if (!hideTooltipRef.current || !shouldAutoDismiss) {
          return;
      }

      const timerID = setTimeout(hideTooltipRef.current, 5000);
      timerIDRef.current = timerID; // we take timerID in a ref
      return () => {
          clearTimeout(timerID);
      };
  }, [shouldAutoDismiss]);
  
 useEffect(() => {
      if (!hideTooltipRef.current || !shouldHideOnNavigation) {
          return;
      }
      const listener = () => {
          hideTooltipRef.current?.();
          if (timerIDRef.current) {
              clearTimeout(timerIDRef.current);
          }
      };
      navigationRef.addListener('state', listener);
      return () => navigationRef.removeListener('state', listener);
  }, [shouldHideOnNavigation]);

Screen.Recording.2024-09-22.at.3.38.16.AM.mov

[daledah]

Updated proposal to cover the navigation logic

[dukenv0307]

Reviewing...

[dukenv0307]

Thanks for your proposals

We have 3 cases that need to fix here

1. Prevent tooltip showing if the modal/popover will be shown
2. If the tooltip is already shown, dismiss it when users open modal/popover
3. If users navigate to Sent, Unread, ... pages, we should preserve the tooltip

so I would like to choose @daledah's proposal

🎀👀🎀 C+ reviewed

[melvin-bot]

Current assignees @luacmartins and @lakchote are eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[abzokhattab]

Thanks for the prompt review @dukenv0307,

Is the expected result that it shouldn't be shown in wide models such as the attachment modal or generally any modal such as the RHP?

I thought it was only specific for the wide modals, like the receipt modal specified in the issue steps, that's why i used the modal?.willAlertModalBecomeVisible not the modal?.isVisible

[dukenv0307]

@abzokhattab I believe we shouldn hide the tooltip if the RHP is open cc @lakchote

[dukenv0307]

The tooltip will be shown at the top of the page (even if the modal is open), so it doesn't make sense if we can interact with the tooltip of the bottom/center pages when the RHP is open

[lakchote]

@abzokhattab I believe we shouldn hide the tooltip if the RHP is open cc @lakchote

I'd say yes, but let's wait for @Expensify/design opinion

[dannymcclain]

@dubielzyk-expensify might have more insight with how the tooltip should work, but from my perspective I think it makes sense to hide it when the RHP is open—especially if it will always sit "on top" of everything else.

[trjExpensify]

I agree with Danny that I think it makes sense to hide it when you open the RHP. 👍

[dubielzyk-expensify]

I agree with @dannymcclain and @trjExpensify

[dukenv0307]

@lakchote We're good to go

[lakchote]

@daledah's proposal LGTM

[daledah]

@dukenv0307 @lakchote PR is ready.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

This issue has not been updated in over 15 days. @lakchote, @luacmartins, @dukenv0307, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[lakchote]

Latest update is here

[luacmartins]

I think this is done, right? We just need to process payment?

[lakchote]

I think this is done, right? We just need to process payment?

Yes, we should process payment.

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[dukenv0307]

@CortneyOfstad Can you process payment here? Thanks

[CortneyOfstad]

@daledah — offer sent here
@dukenv0307 — offer sent here

Please let me know once you both accept and I will get those paid ASAP. Thanks!

[dukenv0307]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other: Internal team

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: Education tooltip was the new feature, we just added fix for the edge case.

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. Yes

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Test:

1. In search page, create a saved search
2. Verify the tooltip is shown
3. Open a receipt expense report
4. Open expense preview
5. Verify that: the tooltip is dismissed.

Do we agree 👍 or 👎

[dukenv0307]

@CortneyOfstad Offer accepted, thank you!

[luacmartins]

Are we still missing anything here?

[lakchote]

Are we still missing anything here?

I think we're good to close?

I'm closing the issue. If we need to reopen it, feel free to do so.

[tgolen]

The checklist has not been completed

[luacmartins]

I don't think we need a separate TC for this. We're adding them all with the project wrap up. I checked off the checkbox in the checklist. Closing.