Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved keyboard navigation/focus handling - Use roving tabindex #799

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

thepuzzlemaster
Copy link

This PR continues where #449 left off in implementing keyboard navigation using arrow keys.

@thepuzzlemaster
Copy link
Author

Okay, I think this is starting to feel pretty good. I updated a few of the behaviors And I added a whole bunch of tests as well.

  • It no longer allows for focus to progress beyond the min/max dates.
  • Clicking on a tile, sets it as the activeTabDate.
  • Navigating Up/Down to a new TileGroup now behaves more expectedly (imo) when the number of tiles is not the same in ever row (decade and century views).

I also started to implement aria role="grid", but ran into some breaking-change roadblocks, since the flex layout doesn't actually include any rows. I do think that would be a nice improvement for the future to get it to more properly implement the role="grid" guidance, since it does follow the grid behaviors. But I think it would be okay to push that improvement out to a future release.

A couple notes/questions:
@wojtekmaj, the tests are fairly redundant, as I am testing the same basic keyboard functionality across all the different arrow keys, and also all the different calendar views. At one point I'd pulled out the shared logic into a helper method which definitely condensed the test file down, but I felt like it sacrificed a little bit in explicitness and readability, so I backed those changes out and left it in its current explicit format. That feels the best to me, but open to suggestions if you'd rather it be formatted differently.

As part of converting this to use a roving tabindex, I had to convert the Flex element from a function component to a forwardRef function component. Since this is an internal implementation detail, I'm not sure if this would be considered a breaking change or not, but figured it'd be worth mentioning in case some people think it might be.

@dutiyesh
Copy link

@wojtekmaj @thepuzzlemaster Hello, is there any information/update by when this can be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants