-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
refactor(v2): make better a11y for tabs #1990
Conversation
Deploy preview for docusaurus-2 ready! Built with commit e602b0a |
Deploy preview for docusaurus-preview ready! Built with commit e602b0a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before I review this, I should let you know of our future plans since you're very involved in Docusaurus 2 development now.
- Infima is a CSS framework and is meant for the base CSS styles which is view library-agnostic. We want it to be usable with Vue and Angular as well.
- We want to come up with
react-infima
which provides Infima-styled React components, and Docusaurus should use them in future instead of writing so many of its own React components like we do now. - This
Tabs
component was meant to be a temporary thing for minimal v1 compatibility. It should really be imported fromimport Tabs from 'react-infima'
in future. I planned to use https://ui.reach.tech/tabs/ and then add Infima classnames to it.
Before I review this PR in-depth, I'd like to get your opinions on what you think about using Reach UI for accessible-ready components so that we don't have to implement the functionality of so many of our own components and can just focus on the appearance.
@yangshun of course, I like the idea of using production-ready, tested, reliable solutions instead of writing your own. Regarding the Reach UI, I just learn from you about this UI, but I enjoy that they are focused on a11y. Although I looking at their Tabs, and for some reason control via Tab does not work, this is strange, maybe I'm doing something not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I removed the removal of the outline styles because they help with a11y actually.
As for using reach-ui, that's an implementation detail which we can sort out in the future. We can ship this first.
Motivation
Tabs cannot be controlled from the keyboard at the moment, this is very bad for a11y.
This PR implements tab switching by Tab/Tab+Shift and arrow keys (left and right).
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Additionally, I added an
onFocus
handler so as not to press the Enter button to open the content of the focusable tab. Because that the contents of all the tabs are already loaded, we can immediately display the contents of focusable tab, it's very cool.For comparison, an example without
onFocus
, where the user needs to press Enter to display the contents of focusable tab. I think in this case it is not very good for UX.