fix(overlay): allow overlay to persist on hover #3706
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
najikahalsema
force-pushed
the
halsema/overlay-hover-handler
branch
from
b9f5e6d
to
8ea6cd2
Compare
Tachometer resultsChromeaction-bar permalink
action-menu permalink
menu permalink
overlay permalink
picker permalink
popover permalink
split-button permalink
tooltip permalink
Firefoxaction-bar permalink
action-menu permalink
menu permalink
overlay permalink
picker permalink
popover permalink
split-button permalink
tooltip permalink
|
Westbrook
reviewed
Oct 6, 2023
Westbrook
reviewed
Oct 6, 2023
| await nextFrame(); | ||
| await nextFrame(); | ||
| // allow "hover" content to close by waiting for its timer to complete | ||
| await aTimeout(400); |
There was a problem hiding this comment.
If the tooltip should be “closed” at this point, could we leverage an sp-closed listener instead?
Westbrook
reviewed
Oct 6, 2023
Westbrook
reviewed
Oct 6, 2023
najikahalsema
force-pushed
the
halsema/overlay-hover-handler
branch
from
3809f27
to
d46f854
Compare
Westbrook
reviewed
Oct 9, 2023
beeduul
reviewed
Oct 9, 2023
| @@ -53,6 +53,7 @@ import { PlacementController } from './PlacementController.js'; | |||
| import styles from './overlay.css.js'; | |||
|
|
|||
| const LONGPRESS_DURATION = 300; | |||
| const HOVER_DELAY = 300; | |||
There was a problem hiding this comment.
how did you arrive at the 300ms duration? what likelihood is this going to be a tunable value?
There was a problem hiding this comment.
Great question! And great catch! I actually just picked an arbitrary number. I think I started with 400 and subconsciously changed it to 300 when I put this variable next to the longpress one 😅 I'll circle back on this and put some rationale behind this number and change it if needed!
Westbrook
approved these changes
Oct 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A timer based approach to allowing hover content to persist so that a user can hover over it.
Related issue(s)
Motivation and context
Blocks Coachmark PR and Rich Tooltip PR. This also matches the expected behaviour for tooltips outlined by WACG.
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.