fix(tab): add aria-disabled depending on disabled prop #4698
Conversation
ghost
requested review from
This comment has been minimized.
This comment has been minimized.
|
Deploy preview for the-carbon-components ready! Built with commit 85dbfcf https://deploy-preview-4698--the-carbon-components.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 85dbfcf |
|
Deploy preview for carbon-components-react ready! Built with commit 85dbfcf https://deploy-preview-4698--carbon-components-react.netlify.com |
|
@vpicone let me know if I'm misunderstanding something but I think my description might have been misleading! These are the violations I'm trying to remove when the tabs are disabled: You totally might know this but the "All content must reside within a HTML5..." violation is always present because of something with storybook and isn't a problem with any component. |
| @@ -160,6 +160,7 @@ export default class Tab extends React.Component { | |||
| {...other} | |||
| tabIndex={-1} | |||
| className={classes} | |||
| aria-disabled={disabled} | |||
There was a problem hiding this comment.
How does this pair with role="presentation"?
There was a problem hiding this comment.
It does remove the DAP violation but as Vince said, since the error is coming from the anchor tag, the aria-disabled should live there so I'm gonna move it!
|
@abbeyhrt totally! Adding aria-disabled tells DAP that the contrast isn't a concern. If you click on those errors you'll see they point to the anchor tag, not the li. Apply the li's have role='presentation' which removes semantic meaning, so we might prefer to add the aria-disabled to the anchor tag where the issues showing up. Also, any reason to use |
|
Deploy preview for carbon-elements ready! Built with commit 79ca5e2 |
|
Deploy preview for the-carbon-components ready! Built with commit 79ca5e2 https://deploy-preview-4698--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 79ca5e2 https://deploy-preview-4698--carbon-components-react.netlify.com |
|
@vpicone I'll switch it the the |
Co-Authored-By: Josh Black <[email protected]>
Co-Authored-By: Josh Black <[email protected]>
|
@abbeyhrt sorry, I forgot they were anchor tags, not buttons. you were spot on with the aria-disabled. Speaking of....why are they anchor tags!? They take an |
|
@vpicone I have no idea why but I'm not touching that lol |
|
@abbeyhrt i don't blame you lol |
| @@ -110,6 +110,19 @@ describe('Tab', () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| it('should set aria-disabled to the value of the disabled prop', () => { | |||
There was a problem hiding this comment.
Would it be better to render it without the disabled prop at all, ensure that it can't find the element, then find the element using the attribute once the prop has been set?
That way we're also testing the case where no prop is provided.
There was a problem hiding this comment.
I'm really new to testing so bear with me, how do I check that it can't find the element? In my head it would be something like expect(getDisabledRegion().prop('aria-disabled')).toBe(undefined); with disabled not set yet.
There was a problem hiding this comment.
Nevermind, figured it out!
Removes DAP violation for disabled state of the Tab Component.
Changelog
New
aria-disabled={disabled}on theliaria-disabledTesting / Reviewing
Go to the Tabs, check the "disabled" knob and run DAP. Verify that this is the only violation you see:
