feat(FormGroupLabelHelp): update markup to match core #10248
Conversation
- don't display <div style="display: contents;" with Popovers
|
Preview: https://patternfly-react-pr-10248.surge.sh A11y report: https://patternfly-react-pr-10248-a11y.surge.sh |
adamviktora
changed the title
adamviktora
requested review from
a team,
mcoker,
nicolethoen and
tlabaj
and removed request for
a team
| typeof ref === 'object' && ref !== null && 'current' in ref && ref.current !== undefined; | ||
|
|
||
| const handleKeyDown = (event: React.KeyboardEvent<HTMLSpanElement>) => { | ||
| if (event.key === 'Enter' && isMutableRef(buttonRef) && buttonRef.current) { |
There was a problem hiding this comment.
We should include the Space key here. Buttons natively will be triggered via Enter/Space, while anchor elements only by Enter, and radio/checkbox inputs only by Space.
There was a problem hiding this comment.
Thanks for the insight! I'll include it
There was a problem hiding this comment.
I just found out the key for Space is " " and we have some hardcoded "Space" keys in the codebase, so I'll replace them with the KeyTypes.Space constant (or the " " string directly in the examples).
| if ([KeyTypes.Space, KeyTypes.Enter].includes(event.key) && isMutableRef(buttonRef) && buttonRef.current) { | ||
| buttonRef.current.click(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
One last little thing. When pressing Space to trigger the popover in this instance, we'll need to prevent the page from scrolling
|
Should there be a codemod written for this change? |
|
@nicolethoen not sure if one will be necessary. The FormGroupLabelHelp was introduced in the v6 branch, and was previously just used to update some examples. Unless we'd want a codemod to check whether |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
LGTM, and I do think the addition of the refs to avoid the popper wrapping div is correct. The wrapping div was an alternate method to get the popper functional, but it's not ideal.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #10158
I am not sure if the last two commits are necessary. I did them so the
<div style="display: contents;">is not present in the markup and there is no better way to hide it from Popover than by passing both thechildrenand thetriggerRefprop to Popover.