-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(Tabs): add ARIA attributes #4970
Conversation
This change adds ARIA attributes to `<Tab>` and `<TabContent>` to better describe their relationship. Fixes carbon-design-system#4968.
Deploy preview for the-carbon-components ready! Built with commit 7d0fbf7 https://deploy-preview-4970--the-carbon-components.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 7d0fbf7 |
Deploy preview for carbon-components-react ready! Built with commit 7d0fbf7 https://deploy-preview-4970--carbon-components-react.netlify.com |
@@ -122,6 +127,7 @@ export default class Tab extends React.Component { | |||
|
|||
render() { | |||
const { | |||
id, |
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.
Should we auto-generate this id if they don't supply one?
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.
Agreed that auto-generation is desirable, but couldn't see a way for <TabContent>
generation code to get such auto-generated ID.
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.
Definitely understand, this is where some of our context experiments have taken us to help coordinate registering these ids.
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.
Right that we need some improvements in this area, I expect other approaches like render-props take into account for that.
@@ -177,7 +184,8 @@ export default class Tab extends React.Component { | |||
onKeyDown(evt); | |||
}} | |||
role="presentation" | |||
selected={selected}> | |||
selected={selected} | |||
aria-controls={`${id}__panel`}> |
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.
Should this be on the a
that has role="tab"
?
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.
Makes sense, fixed.
@@ -158,6 +164,7 @@ export default class Tab extends React.Component { | |||
|
|||
return ( | |||
<li | |||
id={id} |
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.
Should this be on the a
that has role="tab"
?
@asudoh I'm not sure why but the changes are causing two new DAP violations: |
Co-Authored-By: emyarod <[email protected]>
All the review comments from the reviewer have been addressed or not adequate to put in this PR's scope.
Good catch @abbeyhrt! Fixed. |
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.
looks good to me, the DAP violations are resolved now
@joshblack Just in case you have further comments. |
This change adds ARIA attributes to `<Tab>` and `<TabContent>` to better describe their relationship. Fixes carbon-design-system#4968.
This change adds ARIA attributes to
<Tab>
and<TabContent>
to better describe their relationship.Fixes #4968.
Changelog
New
aria-controls
to<Tab>
.aria-labelledby
to<TabContent>
.Testing / Reviewing
Testing should make sure React tabs component is not broken.