feat(Popover) add option to trigger popover on hover #9283
Conversation
| } | ||
| }; | ||
|
|
||
| const onMouseEnter = (event: any) => { |
There was a problem hiding this comment.
I need some help with typing the event parameter, I tried couple things but could not come up with what type and type conversions to use. It'd be great if anyone could help.
|
Preview: https://patternfly-react-pr-9283.surge.sh A11y report: https://patternfly-react-pr-9283-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
This is looking good so far! Had some comments below, additionally some other comments below for @andrew-ronaldson
- if we're offering a Popover to be triggered on hover, we should also support it being triggered on focus for users that navigate with keyboard. Otherwise they won't be able to view the Popover content (assuming this is a choice between hover or click).
- Is it expected that a hover Popover won't contain any content that requires/offers interaction? I.e. buttons/links, scrollable containers that require scrolling, etc. If there is interactive content within this type of Popover, it could lead to a11y issues. Tying into the above bullet, if I focus a trigger and that causes a Popover to open, focus would have to be placed inside the Popover when I press Tab if there's interactive content; that might require (us or consumers) to manage focus if a Popover is appended to the end of the document.
| const showTimerRef = React.useRef(null); | ||
| const hideTimerRef = React.useRef(null); | ||
| const popoverRef = React.useRef(null); | ||
| const hideShowTime = variant === 'hover' ? animationDuration : 0; |
There was a problem hiding this comment.
This will probably have to be updated once #9339 goes in, cc @wise-king-sullyman
There was a problem hiding this comment.
Yeah it will if it's needed, though I'm not sure it will be? In #9339 I already have the entryDelay and exitDelay props enabling consumer side customization of those timeout values.
dominik-petrik
force-pushed
the
popover-hover-variant
branch
from
6d413f0 to
8a0d617
Compare
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Depending on the order things are merged, may need another update if 9339 goes in first
thatblindgeye
force-pushed
the
popover-hover-variant
branch
from
8a0d617 to
2d48c15
Compare
|
Opened #9441 to resolve the tests that were skipped in this PR |
|
Looks good! Just wonder about how short the delay is between hover and popover showing up - as long as that delay time is customizable, I approve! |
* feat(Popover) add option to trigger popover on hover * trigger popover on focus, minor changes * Removed unnecessary hideShowTime var * Skipped failing tests --------- Co-authored-by: Eric Olkowski <thatblindgeye@gmail.com>
6 participants
What: Closes #7072