fix(Select): Footer keyboard bugs#6844
Conversation
nicolethoen
left a comment
There was a problem hiding this comment.
Just a quick question. Otherwise - as far I was able to hammer on it, it seems to work :)
| direction={direction} | ||
| footer={<Button variant="link" isInline>Action</Button>} | ||
| footer={ <> | ||
| <Button tabIndex={1} variant="link" isInline> |
There was a problem hiding this comment.
is this a requirement to add a tabIndex? should we call that out in the docs?
There was a problem hiding this comment.
Upon testing - it doesn't seem to be a requirement - but it in an interesting choice. Could you explain the goal?
There was a problem hiding this comment.
The keyboard navigation seems to skip over the "Miss." option and either lands on "Mrs" or "Mrs". That is, when I click on the select first, then navigate using the up/down arrow.
It's not clear that tab was required to navigate to the footer. I was expecting to use the up/down arrow keys to navigate to the footer (i.e., considering I was already navigating within the menu).
| footer={ <> | ||
| <Button variant="link" isInline> | ||
| Action | ||
| </Button> | ||
| </>} |
There was a problem hiding this comment.
Curious about the empty tag, I recall this did not work with PF docs. Has it been enabled?
That said, an empty tag doesn't seem necessary here.
There was a problem hiding this comment.
Correct. I was testing with two buttons and the empty tag was a left over.
I am not apply to reproduce the issue with skipping the "Miss"
wise-king-sullyman
left a comment
There was a problem hiding this comment.
There are some items in here that I think would ideally be better abstracted into separate functions, but given that similar patterns are followed throughout this component I'm not willing to hold up approval for it. Everything seems to work well based on my testing.
thatblindgeye
left a comment
There was a problem hiding this comment.
In terms of the changes made to fix the issue at hand, things look good/work well from the preview build! I would also agree with Austin's comment, but considering the issue has been fixed I will put an approval in as well.
jessiehuff
left a comment
There was a problem hiding this comment.
This seems pretty good. I feel uncertain about Tab navigating the checkbox select examples, but I noticed that it's also on the surge preview site so it's likely unrelated to this PR. Also, talking to Jenn, she thinks that's probably fine given that checkboxes can be seen as different to users than normal menu items so having both might be ok. This one seems subjective.
In terms of clicking on the toggle and then using arrow keys, I don't think that's been implemented in Select and is separate, right? I remember creating an issue for that here. It seems to work ok otherwise for keyboard and VO.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6485
Additional issues:
I also addressed some keyboard interaction issues for:
*Note: demos were added for testing purposes only