feat(core): Popover positioning #1032
Conversation
Pull Request Test Coverage Report for Build 3672
💛 - Coveralls |
|
PatternFly-React preview: https://1032-pr-patternfly-react-patternfly.surge.sh |
|
@christiemolloy I have some questions regarding the popover.
@jgiardino Should the user be able to tab into the popover? What should the flow here be? |
For the popover, I would trap focus like we do for modal (#1011) and use the same keyboard interaction that we use for the modal. The expected keyboard interaction for modal is described in issue #561 Let me know if you have questions about this. The documentation regarding where to place initial focus in the modal/popover is quite long. |
|
Thanks @jgiardino , I'll focus trap similar to the modal |
|
@jschuler to your second point about overflow of the popover in your current view, I think thats definitely the right approach to flip the positioning, and that would be the case for all 4 positions. And to your first point there should be an animation tied to the popover. @mcarrano can you clarify this behavior? Also what does the "Keep in View" check do? |
|
I think this looks great @jschuler . I like the approach to flip the orientation to keep the popover in view. As to the question about animation, I like what you've done. We really never settled on a standard for animating objects that open and close. Is there a good way to package and reuse this so we don't have inconsistent effects for various components that require animation? |
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
Looks good Initially we hd said we would use popper. Curios why we are using tippy?
|
@tlabaj tippy is powered by popper, but also does a lot of things we'd still have to do if we used popper straight up |
| lazy | ||
| trigger={handleEvents ? 'click' : 'manual'} | ||
| isVisible={isVisible} | ||
| hideOnClick={shouldHideOnClick()} |
There was a problem hiding this comment.
do you really want to call the function here?
There was a problem hiding this comment.
Yes, it basically maps the prop hideOnOutsideClick with values of true or false to true and 'toggle'
| <Button onClick={onClose} variant="plain" aria-label="Action"> | ||
| const PopoverCloseButton = ({ onClose, ...rest }) => ( | ||
| <div className={css(styles.popoverClose)} {...rest}> | ||
| <Button onClick={onClose} variant="plain" aria-label="Close"> |
There was a problem hiding this comment.
Maybe add the aria-label as prop that can be applied to a button for localization purposes.
There was a problem hiding this comment.
Good point, I'll do that
10 participants
Add popover positioning support
Closes: #487