-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add show on hover to info popover #48990
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~60 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~3166 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~1427 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should make this change.
It is inconsistent with the use of the info popover everywhere else in Calypso. And, by hovering, there are accessibility issues... if the user does not carefully move their mouse pointer to the "Learn more" link in the popover, the whole thing disappears. This can be very frustrating. (Though, it could probably be made less likely by increasing the timeout before auto-dismissing the popover.)
And by putting this support in the common InfoPopover
component, we are inviting other developers to start using this sometimes, which will lead to further inconsistent usage. Either this component should always work with hover, everywhere, or never, in my opinion.
It's not a big deal if the user doesn't realize to click on the info button in this case, as clicking on the entire "Store" menu item will take them to a page that has a large notice at the top of it providing even more detail than this popover does.
I've tested the change and it works, including on iOS (both with touch and trackpad). And existing use of the info popover elsewhere seems to be functioning properly still too.
/cc @elizaan36 Do you feel strongly that we should still make this change at this point, even if inconsistent with usage elsewhere in Calypso?
Hey @mattsherman Yes let's make sure the tooltip stays active while the user hovers over it in order to access the learn more link. As for requiring the click, I'm a little unclear as to why this is being brought up now after many design specs that specify "tooltip on hover", but I don't think it's a problem to diverge from Calypso for this temporary feature. That said, I don't think this functionality should be included in the common component for usage elsewhere. Imo, it's more of a problem to require users to think about if they need to click in order to open the tooltip, and will they accidentally click on the nav. |
Hover to show the tooltip is inconsistent with the rest of Calypso, I debated with myself whether it was appropriate to diverge the behaviour especially since we (Isotope) don't work in Calypso much and don't have the full context over the use of this component. However I'm ok with deferring to @elizaan36 on this. I don't see a big problem with leaving the functionality in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not a fan of making this change, but I also do not want to hold up a change that others believe should be made... this is not an change I feel strongly enough about to actively block. There are pros and cons to it, and I can see the argument for making the change.
I'd like to see the timeout increased a bit to make the auto-dismiss a bit more forgiving for users that don't precisely position the cursor when going to click on a link in the popover.
I'm approving this pre-emptively, based on my review of the code and testing conducted yesterday. The code changes seem solid and well done. It's just the functionality in the context of the existing system that I disagree with.
} | ||
|
||
this.setState( { showPopover: false }, this.recordStats ); | ||
}, 100 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try bumping this up a bit and seeing if it still feel okay? It would help prevent the popover from auto-dismissing when the user accidentally moves the cursor briefly outside of the info button.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
250ms seems to be the sweet spot, thanks for the review @mattsherman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels much better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with increased auto-dismiss timeout... much better! Thanks for making the change.
Changes proposed in this Pull Request
Testing instructions
Fixes #48982