-
Notifications
You must be signed in to change notification settings - Fork 377
feat(Popper): add mouse events for popper content #7399
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
Conversation
|
Preview: https://patternfly-react-pr-7399.surge.sh A11y report: https://patternfly-react-pr-7399-a11y.surge.sh |
jessiehuff
left a comment
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 don't off the top of my head and would need to research this. But what's there now feels pretty good. |
|
@jessiehuff I don't know either, it's not anything we've defined in core. Our flyout menu may also have an exit delay. |
nicolethoen
left a comment
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.
LGTM
|
@jessiehuff I know at one point Zack had updated our delays for tooltip. It was changed to 0? |
|
@jessiehuff @mcarrano here is the tool tip PR that changed the delays previously. #5746 |
tlabaj
left a comment
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 may be considered a breaking change.
|
@tlabaj if this would be considered a breaking change I can revert the |
I don't know that it would be considered breaking. @mcarrano with the tooltip change you said the exit delay update was warranted. How do you feel about a change here? |
|
I think I'm confused about what this PR is doing. Is Popper a component that is exposed to the consumer? Do you mean Popover? |
|
This PR mainly resolves the issue of keeping tooltips active when a mouse hovers over the actual tooltip contents rather than only the trigger. Tooltip just uses Popper under the hood, so I had to make an update to Popper to implement this feature. I had also updated the |
|
If the exit delay is zero, would the user be able to hover over the tooltip or would it interfere (like Eric's example above shows)? I'm wondering if the tooltip would disappear before they're able to get to it? Ideally for Level AA compliance, tooltips should be persistent on hover. (WCAG 1.4.13) |
|
You are able to hover the tooltip with an exitDelay of 0, but since the entryDelay is longer it causes that effect of the tooltip starting to hide and then re-appear when you hover it. Setting both entry and exit delays to the same number, or making the exit delay longer, resolves that visual issue. |
|
I don't know why we set the exit delay to 0 in the past, but seems to me that the entry and exit delays should be consistent. Can we just make them the same? Do you see any problem with this @tlabaj ? |
|
tlabaj
left a comment
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.
lgtm
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #1457
Preview build - Tooltip
Two new mouse events were added to the Popper component, both to resolve the above issue and for other components that use Popper to potentially utilize these events.
I also updated the default
exitDelayfor the tooltip with an explanation. Below is a video for the behavior whenexitDelay={0}(technically it works). I only chose a value of "300" to match theentryDelay, but if anyone has a default they'd prefer it can be tweaked.Tooltip.with.0.Exit.Delay.mov
Additional issues: