Tooltip gets stuck on the page fix #12572

getusha · 2022-11-08T22:15:18Z

Details

#12025 (comment)

Fixed Issues

$ [PAID] [$1000] Bug: Tooltip gets stuck on the page reported by @Puneet-here #12025

PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Decrease the screen size
Hover over any contact at chat navigator
When the tooltip appears open the chat
Hover at back button until the tooltip shows up then press back
The Tooltip should disappear after clicking on the chat / pressing back and only re-appear when hovering clickable items

Verify that no errors appear in the JS console

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

20221109_215653.mp4

iOS

Android

github-actions · 2022-11-08T22:15:36Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

getusha · 2022-11-09T14:30:35Z

recheck

PauloGasparSv · 2022-11-09T14:49:33Z

Hey @getusha, hope you are doing well.
Sorry for not commenting here sooner!

Do you mind adding evidences for Desktop too? Also checking the missing items of your PR Author Checklist section? (I know this is just a Web and Desktop issue but we need to check all of them)
Also, in the Fixed Issues section, can you change the content to the URL itself after the dollar sign like this:

$ [PAID] [$1000] Bug: Tooltip gets stuck on the page reported by @Puneet-here #12025
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Do you mind adding a couple more items to the Tests describing what behavior should be happening after the click? I think it would be something like "The clickable mouse tooltip should disappear after clicking on the chat and only re-appear when hovering clickable items"
One last thing, can you comment in the QA Steps section something like "Same as Tests" so no one thinks there are NO QA steps?

mountiny · 2022-11-09T15:13:22Z

@getusha Great to see your first PR! For your next one, please make sure to link the issue correctly as described in the template, like so: $ https://github.com/Expensify/App/issues/12025

Thank you!

getusha · 2022-11-09T15:15:04Z

@PauloGasparSv Thanks, I will add the listed requests

getusha · 2022-11-09T16:31:48Z

Done @PauloGasparSv
Sorry for the delay.

getusha · 2022-11-10T17:39:14Z

@PauloGasparSv @rushatgabhane @chiragsalian
Can we get this reviewed? Thank you.

rushatgabhane

@chiragsalian LGTM! 🎉

Web

Screen.Recording.2022-11-11.at.1.06.47.AM.mov

mWeb

Screen.Recording.2022-11-11.at.1.10.55.AM.mov

iOS

NA

Android

NA

Desktop

Screen.Recording.2022-11-11.at.1.12.49.AM.mov

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

src/components/Hoverable/index.js

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

src/components/Hoverable/index.js

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

getusha · 2022-11-10T19:51:48Z

Thank you @rushatgabhane
All set?

rushatgabhane · 2022-11-10T20:04:50Z

@getusha it's @chiragsalian / @PauloGasparSv's to review

PauloGasparSv

LGTM! Only uploading footage from web and desktop.
All yours @chiragsalian

PR Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Web

Web.mov

Desktop

Desktop.mov

rushatgabhane · 2022-11-11T11:38:19Z

@chiragsalian let's get this merged today :)

chiragsalian

LGTM, waiting for GH actions to finish.

src/components/Hoverable/index.js

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

rushatgabhane · 2022-11-11T18:00:52Z

@getusha please find a fix for it if you can, meanwhile I'll try to do the same

rushatgabhane · 2022-11-11T18:22:17Z

Created a fix PR - #12677

@getusha can you please see if it'll cause another regression - #6083

getusha · 2022-11-11T18:23:54Z

Sure @rushatgabhane working on it. will update you

getusha · 2022-11-11T18:29:01Z

Looks like we have the resetsOnClickOutside prop applied inside ReportActionItem's Hoverable we need to pass a new prop to check if the prop is from the Tooltip

rushatgabhane · 2022-11-11T18:37:33Z

@getusha we can also remove resetsOnClickOutside from ReportActionItem's Hoverable, right?

getusha · 2022-11-11T18:43:52Z

That don't seem like a good idea, instead we should add isTooltipComponent prop to the Hoverable inside the Tooltip
And
if(this.props.resetsOnClickOutside && this.props.isTooltipComponent) {

getusha · 2022-11-11T18:50:15Z

Should I raise new PR for this?

getusha · 2022-11-11T18:53:56Z

I am about to raise the new PR

rushatgabhane · 2022-11-11T18:55:34Z

sorry for the deleted comments.

That don't seem like a good idea

@getusha im curious to know why not?

getusha · 2022-11-11T18:55:59Z

There might be something related to it?

getusha · 2022-11-11T18:58:02Z

Can you raise the new PR @rushatgabhane ?

rushatgabhane · 2022-11-11T19:01:56Z

@getusha it's here - #12677

rushatgabhane · 2022-11-11T19:03:07Z

There might be something related to it

That makes sense, my PR might cause #5972 again.

But in my tests so far, it doesn't cause it. So i think we're good to go.

getusha · 2022-11-11T19:04:01Z

Great, I think we should go with that.
Will check id more regressions occur

getusha · 2022-11-11T19:05:19Z

Well so let's take the new prop approach

getusha · 2022-11-11T19:09:30Z

Here is the new PR #12681
@rushatgabhane

getusha · 2022-11-11T19:10:56Z

We can remove the other one?

rushatgabhane · 2022-11-11T19:12:35Z

@getusha sorry that i wasn't clear enough.

I think we should go with #12677

because it doesn't need the tooltipComponent prop, so it's cleaner.

getusha · 2022-11-11T19:16:34Z

@rushatgabhane
Great is that not causing additional bugs?

rushatgabhane · 2022-11-11T19:19:20Z

@getusha i thought my pr would cause #5972

but while testing, it wasn't causing.

i could be wrong, so please correct me

OSBotify · 2022-11-15T21:58:59Z

🚀 Deployed to staging by @chiragsalian in version: 1.2.28-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	failure ❌
🕸 web 🕸	success ✅

OSBotify · 2022-11-15T23:40:59Z

🚀 Deployed to staging by @chiragsalian in version: 1.2.28-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-11-17T07:38:01Z

🚀 Deployed to production by @roryabraham in version: 1.2.28-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

mollfpr · 2023-01-17T03:57:13Z

Comment regarding #13146 BZ checklist.

This PR has been identified for introducing the #13146 issue. This is an edge case, where resetting the hoverable state click on Chrome the hoverable state will be active again but in Safari the hoverable state is active after some movement.

The explanation and the example can be checked on this comment.

Thanks!

getusha added 2 commits November 9, 2022 00:28

add resetsOnClickOutside prop to Tooltip component

5975e85

resetHoverStateOnOutsideClick tweaks

1faea7a

getusha requested a review from a team as a code owner November 8, 2022 22:15

melvin-bot bot requested review from PauloGasparSv and removed request for a team November 8, 2022 22:15

mountiny assigned getusha Nov 9, 2022

mountiny requested review from rushatgabhane and chiragsalian November 9, 2022 15:12

rushatgabhane previously approved these changes Nov 10, 2022

View reviewed changes

rushatgabhane reviewed Nov 10, 2022

View reviewed changes

src/components/Hoverable/index.js Outdated Show resolved Hide resolved

Update src/components/Hoverable/index.js

a827059

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

getusha dismissed rushatgabhane’s stale review via a827059 November 10, 2022 19:48

rushatgabhane reviewed Nov 10, 2022

View reviewed changes

src/components/Hoverable/index.js Outdated Show resolved Hide resolved

Update src/components/Hoverable/index.js

e9d3b87

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

rushatgabhane previously approved these changes Nov 10, 2022

View reviewed changes

PauloGasparSv previously approved these changes Nov 10, 2022

View reviewed changes

chiragsalian previously approved these changes Nov 11, 2022

View reviewed changes

rushatgabhane reviewed Nov 11, 2022

View reviewed changes

src/components/Hoverable/index.js Outdated Show resolved Hide resolved

getusha dismissed chiragsalian’s stale review via 4ac7908 November 11, 2022 15:34

Update src/components/Hoverable/index.js

4ac7908

Co-authored-by: Rushat Gabhane <rushatgabhane@gmail.com>

rushatgabhane mentioned this pull request Nov 11, 2022

Fix unclickable context menu #12677

Merged

92 tasks

rushatgabhane mentioned this pull request Nov 11, 2022

Clicking on context menu item does nothing. #12685

Closed

OSBotify mentioned this pull request Nov 15, 2022

Deploy Checklist: New Expensify 2022-11-15 #12753

Closed

31 tasks

melvin-bot bot mentioned this pull request Nov 17, 2022

[PAID] [$1000] Bug: Tooltip gets stuck on the page reported by @Puneet-here #12025

Closed

allroundexperts mentioned this pull request Nov 30, 2022

[$8000] Mac / Safari - Copy to clipboard Tooltip doesn't show copied on clicking the clipboard icon. #13146

Closed

BeeMargarida mentioned this pull request Jun 30, 2023

[HOLD for payment 2023-07-14] [$1000] The tooltip disappears when the "Copy to clipboard" option is clicked multiple times. #19693

Closed

6 tasks

Tooltip gets stuck on the page fix #12572

Tooltip gets stuck on the page fix #12572

Conversation

getusha commented Nov 8, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

github-actions bot commented Nov 8, 2022 • edited Loading

getusha commented Nov 9, 2022

PauloGasparSv commented Nov 9, 2022

mountiny commented Nov 9, 2022 • edited Loading

getusha commented Nov 9, 2022

getusha commented Nov 9, 2022

getusha commented Nov 10, 2022

rushatgabhane left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

getusha commented Nov 10, 2022

rushatgabhane commented Nov 10, 2022

PauloGasparSv left a comment • edited Loading

Choose a reason for hiding this comment

PR Reviewer Checklist

Web

Desktop

rushatgabhane commented Nov 11, 2022

chiragsalian left a comment • edited Loading

Choose a reason for hiding this comment

rushatgabhane commented Nov 11, 2022 • edited Loading

rushatgabhane commented Nov 11, 2022

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022 • edited Loading

rushatgabhane commented Nov 11, 2022

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022

rushatgabhane commented Nov 11, 2022

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022

rushatgabhane commented Nov 11, 2022

rushatgabhane commented Nov 11, 2022

getusha commented Nov 11, 2022 • edited Loading

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022

getusha commented Nov 11, 2022

rushatgabhane commented Nov 11, 2022

getusha commented Nov 11, 2022

rushatgabhane commented Nov 11, 2022 • edited Loading

OSBotify commented Nov 15, 2022

OSBotify commented Nov 15, 2022

OSBotify commented Nov 17, 2022

mollfpr commented Jan 17, 2023

getusha commented Nov 8, 2022 •

edited

Loading

github-actions bot commented Nov 8, 2022 •

edited

Loading

mountiny commented Nov 9, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

PauloGasparSv left a comment •

edited

Loading

chiragsalian left a comment •

edited

Loading

rushatgabhane commented Nov 11, 2022 •

edited

Loading

getusha commented Nov 11, 2022 •

edited

Loading

getusha commented Nov 11, 2022 •

edited

Loading

rushatgabhane commented Nov 11, 2022 •

edited

Loading