fix(Select-next,Dropdown-next): updated logic for keyboard interaction#8496
Conversation
|
Preview: https://patternfly-react-pr-8496.surge.sh A11y report: https://patternfly-react-pr-8496-a11y.surge.sh |
thatblindgeye
force-pushed
the
iss8124_nextComponents_spaceKey
branch
from
bc3c6d8 to
66791bf
Compare
thatblindgeye
force-pushed
the
iss8124_nextComponents_spaceKey
branch
from
66791bf to
4fd250d
Compare
| /** Set the value of data-ouia-safe. Only set to true when the component is in a static state, i.e. no animations are occurring. At all other times, this value must be false. */ | ||
| ouiaSafe?: boolean; | ||
| /** @beta The variant of a non-checkbox menu. Determines whether one or more items can be selected. */ | ||
| selectVariant?: 'single' | 'multi'; |
There was a problem hiding this comment.
Can the description mention specifically that the role and aria props are being manipulated by this prop? Maybe I'm thinking too much of the original select but currently it sounds like to me that the prop should be changing functionality / handling of selection when it doesn't.
Everything else lgtm.
There was a problem hiding this comment.
Yes agree with Katie. The prop name is a little misleading. I am trying to think i there is a better name...
There was a problem hiding this comment.
Yeah the prop name I wasn't totally sure on. ariaSelection could maybe fit a little more, but there are non ARIA attributes being set. selectionType is another one, though may not be better than selectVariant.
I can push an update to the description and see if that would help a bit, though definitely open to updating the prop name as well if we can land on one that's more fitting.
There was a problem hiding this comment.
Sounds good for the description. Name-wise I haven't been able to think of a better fitting name. roleAriaType maybe, but that's a bit unclear still. roleAndAriaType?
There was a problem hiding this comment.
Maybe using aria-multiselectable as the prop for Menu and having to pass in true or false? If the prop is undefined, the Menu isn't a non-checkbox select menu, otherwise true or false could still set the role and ARIA attributes, with the actual boolean setting the aria-multiselectable attribute itself. Or maybe ariaSelectionType to keep the "single" | "multi" typing.
There was a problem hiding this comment.
Maybe for Menu we can use aria-multiselectable and role as direct props? Then in Select/Dropdown next we can wrap them in an ariaSelectionType kind of property?
There was a problem hiding this comment.
That could work... For role we could have it be to set the MenuList role, so either listbox or menu, with that determining the role of MenuItem (if role="listbox" then MenuItem would have role="option" set).;
@tlabaj wdyt?
There was a problem hiding this comment.
Yes, I think that would be much more intuitive. Good idea.
There was a problem hiding this comment.
@kmcfaul @tlabaj updated! Had to also add a role prop for Select Next so that a checkbox select could have role="menu" set on it (with the default being role="listbox")
@nicolethoen would you mind taking another pass in VO?
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
VO and keyboard work as expected! LGTM
thatblindgeye
force-pushed
the
iss8124_nextComponents_spaceKey
branch
from
99833a7 to
84ffa14
Compare
nicolethoen
left a comment
nicolethoen
left a comment
There was a problem hiding this comment.
VO sounds good to me
|
|
||
| return ( | ||
| <Menu onSelect={onSelect} activeItemId={activeItem} selected={selectedItem}> | ||
| <Menu role="listbox" onSelect={onSelect} activeItemId={activeItem} selected={selectedItem}> |
There was a problem hiding this comment.
Can we note the use of listbox in the text above these two updated examples? since erin has written such beautiful documentation for our Menu examples 😊
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
6 participants
What: Closes #8124
Dropdown next preview
Select next preview
For the additional issue noted in the link above regarding adding
role="option"to the underlying MenuItem, arole="listbox"andaria-multiselectablewould need to be added to MenuList andaria-selectedto MenuItem in order to make things more accessible.I pushed one possible implementation where a consumer would just need to pass in
selectVariantwith a value of "single" or "multi" (works for either Next Select or just a plain Menu component), with that adding all the necessary attributes under the hood.The alternative wouldn't really change anything for Next Select (consumer would still just have to pass in a prop to determine whether it's a single or multi select and everything else would be done under the hood), but when using the Menu components themselves to set something up similar to our "Option single select" and "Option multi select" examples, the
rolewould need to be manually passed into MenuItem and MenuList.Additional issues: