-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: add aria label and aria disabled #691
Conversation
@@ -30,6 +31,8 @@ const Button = forwardRef( | |||
<StyledButton | |||
ref={ref} | |||
disabled={disabled} | |||
aria-disabled={disabled || undefined} |
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.
Isn't this enough?
aria-disabled={disabled || undefined} | |
aria-disabled={disabled} |
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.
I did a test to see what the difference would be when disabled
is false
or undefined
With: aria-disabled={disabled || undefined}
And with: aria-disabled={disabled}
Using aria-disabled={disabled}
the aria-disabled attribute receives false, on the other hand, using aria-disabled={disabled || undefined}
the aria-disabled attribute does not even appear.
I'm thinking what the best way would be, what do you think @MicaelRodrigues?
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.
@MicaelRodrigues I talked to the team and we realized that it would be good to keep aria-disabled={disabled || undefined}.
It's more common the aria-disabled
not to appear when it's false
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.
I did a test to see what the difference would be when
disabled
isfalse
orundefined
With:
aria-disabled={disabled || undefined}
And with:
aria-disabled={disabled}
Using
aria-disabled={disabled}
the aria-disabled attribute receives false, on the other hand, usingaria-disabled={disabled || undefined}
the aria-disabled attribute does not even appear.I'm thinking what the best way would be, what do you think @MicaelRodrigues?
@MicaelRodrigues I talked to the team and we realized that it would be good to keep
aria-disabled={disabled || undefined}.
It's more common the
aria-disabled
not to appear when it'sfalse
@matheushdsbr I understood all that... ;)
but if we want the default to be undefined
we should just update the default prop value for disabled
to undefined
.
Take a look at the chat bellow. There are slight differences between false
and undefined
use cases. A Button
user may want to explicitly set aria-disabled
to false
and with the current implementation he can't.
https://chat.openai.com/c/12783a4b-bd20-4823-bc2c-62b4fdb4f2f7
WDYT? :)
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.
Thanks for the tips, let's continue with changing disabled
in defaultProps
to undefined
Description 📄
Add accessible attributes: aria-disabled and aria-label
Platforms 📲
Type of change 🔍
How Has This Been Tested? 🧪
[Enter the tips to test this PR]
Checklist: 🔍