Open m-natarajan opened 3 weeks ago
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to @mallenexpensify (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.
Nodebrute
commented
3 weeks ago First QAB missing tooltip for new user
This issue began after the changes in this PR. In this PR, we updated EducationalTooltip by adding a new variable, shouldShow, which included two additional checks: !modal?.willAlertModalBecomeVisible && !modal?.isVisible.
https://github.com/Expensify/App/blob/a84cb3181161b1d3fdb1efcf9c2e375884606667/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L26
The problem arises because when we open the FAB, willAlertModalBecomeVisible is set to true, which causes shouldShow to be evaluated as false, preventing the tooltip from displaying.
We can change the condition so when isPopover is true we ignore these !modal?.willAlertModalBecomeVisible && !modal?.isVisible
https://github.com/Expensify/App/blob/a84cb3181161b1d3fdb1efcf9c2e375884606667/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L26
we can do something like this
const shouldShow = shouldRender && (modal?.isPopover || (!modal?.willAlertModalBecomeVisible && !modal?.isVisible));Note: This is just pseudo code. We can achieve the same results by passing props
We can remove !modal?.willAlertModalBecomeVisible from here
https://github.com/Expensify/App/blob/a84cb3181161b1d3fdb1efcf9c2e375884606667/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L26
bernhardoj
commented
3 weeks ago The QAB toolltip doesn't show for new user.
This happens after https://github.com/Expensify/App/pull/49682 where we prevent the tooltip to show in a modal. The issue that they are trying to solve is that the tooltip doesn't hide when we open a modal (or RHP). https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L26
But using that condition means that the tooltip isn't allowed to show in a modal.
Because we want to close the tooltip when a modal will show, we need to change the logic a bit. What we should do instead is, if a modal is previously not shown, but now is visible, then close the tooltip. We can do that inside this onyx subscription. https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L32-L41
let prevModal: Modal;
const unsubscribeOnyxModal = onyxSubscribe({
key: ONYXKEYS.MODAL,
callback: (modal) => {
if (modal === undefined) {
return;
}
if (!prevModal) {
prevModal = modal;
} else if ((!prevModal.willAlertModalBecomeVisible && modal.willAlertModalBecomeVisible) || (!prevModal.isVisible && modal.isVisible)) {
clearTimeout(showTimer.current);
clearTimeout(autoCloseTimer.current);
closeTooltip();
}
},
});
// If we want to replace above with useOnxy hooks (we use the above logic so it will re-render based on modal only when visible)
// const [modal] = useOnyx(ONYXKEYS.MODAL);
// const prevModal = usePrevious(modal)
// useEffect(() => {
// if ((!prevModal?.willAlertModalBecomeVisible && modal?.willAlertModalBecomeVisible) || (!prevModal?.isVisible && modal?.isVisible)) {
// clearTimeout(showTimer.current);
// clearTimeout(autoCloseTimer.current);
// closeTooltip();
// }
// }, [modal, prevModal, closeTooltip]);
And when we close the tooltip, we need to make sure we cancel both the timeout of show and auto close/dismiss. https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L79-L85 https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L93-L99
That's why we need to store it on a ref so we can use it later. Then, we can remove this shouldShow and this logic.
https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L26
https://github.com/Expensify/App/blob/10454f06334ae3d07d4a28a81c5c79f3d2112c53/src/components/Tooltip/EducationalTooltip/BaseEducationalTooltip.tsx#L73-L77
Also, replaces all shouldShow with shouldRender back.
melvin-bot[bot]
commented
3 weeks ago Job added to Upwork: https://www.upwork.com/jobs/~021853953365086766977
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @rayane-djouah (External)
mallenexpensify
commented
3 weeks ago @ishpaul777 can you provide more details plz?
Sign up with new account
Which do you select here?
Open FAB, complete scan receipt flow
Track expense or Submit expense?
Thx
ishpaul777
commented
3 weeks ago Track expense or Submit expense?
Submit expense, not only scan receipt but the any of the submit flow will work
mallenexpensify
commented
3 weeks ago Was able to reproduce
Video
https://github.com/user-attachments/assets/6e8a67bc-2da1-4a06-9002-168999e0476c
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to @sakluger (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.
mallenexpensify
commented
3 weeks ago @sakluger , I'm off the next week, can you please keep an eye on this issue til I'm back? Thx
sakluger
commented
3 weeks ago @rayane-djouah could you please check the two proposals we got already to see if they would work? Thanks!
rayane-djouah
commented
3 weeks ago Reviewing 👀
rayane-djouah
commented
3 weeks ago @Nodebrute Thank you for the proposal. However, the proposed solution will not be effective in scenarios where a tooltip is already displayed and a modal (popover) containing another tooltip is opened subsequently:
https://github.com/user-attachments/assets/e7ad78bf-937e-4a3f-ab81-1c71cb79c4ab
Also, the tooltip will not be closed when we click on the three dots menu in saved searches:
https://github.com/user-attachments/assets/049364b1-bf04-4323-a666-a81fb9530b37
rayane-djouah
commented
3 weeks ago @bernhardoj Thank you for your proposal. I agree with your root cause analysis and the solution you've suggested.
:ribbon::eyes::ribbon: C+ reviewed
melvin-bot[bot]
commented
3 weeks ago Triggered auto assignment to @pecanoro, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
sakluger
commented
2 weeks ago Hey @pecanoro, could you please check the recommended proposal?
pecanoro
commented
2 weeks ago Assigning @bernhardoj to the issue!
melvin-bot[bot]
commented
2 weeks ago 📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
bernhardoj
commented
2 weeks ago PR is ready
cc: @rayane-djouah
melvin-bot[bot]
commented
2 days ago Reviewing label has been removed, please complete the "BugZero Checklist".
melvin-bot[bot]
commented
2 days ago The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.67-9 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-12-05. :confetti_ball:
For reference, here are some details about the assignees on this issue:
melvin-bot[bot]
commented
2 days ago @rayane-djouah @mallenexpensify @rayane-djouah The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]
rayane-djouah
commented
1 day ago [x] Classify the bug:
[x] 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: https://github.com/Expensify/App/pull/49682/files#r1863654116
[x] 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
[x] 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.
[ ] [@mallenexpensify] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.
Link to issue:
#### Precondition:
- N/A
#### Test:
1. Log in as a new user.
2. Submit a new expense to any user.
3. Press the FAB (Floating Action Button).
4. Verify that a tooltip is shown for the QAB (Quick Access Bar).
5. Verify that the tooltip auto-hides after 5 seconds.
Do we agree 👍 or 👎
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.57-0 Reproducible in staging?: y Reproducible in production?: y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @ishpaul777 Slack conversation (hyperlinked to channel name): Expensify-bugs
Action Performed:
Sign up with new account Open FAB, complete scan receipt flow Open FAB again
Expected Result:
you see a QAB with tooltip
Actual Result:
you see a QAB without tooltip
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/user-attachments/assets/2ce97674-a251-4918-a6d0-d20ab2935310
Expected tooltip:
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @mallenexpensify