-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
[a11y] Set aria-label explicitly #1594
Conversation
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.
It'd be good to have a test to cover my comment.
src/components/DateInput.jsx
Outdated
@@ -217,7 +220,7 @@ class DateInput extends React.PureComponent { | |||
focused && styles.DateInput_input__focused, | |||
disabled && styles.DateInput_input__disabled, | |||
)} | |||
aria-label={placeholder} | |||
aria-label={ariaLabel} |
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.
we'd need to retain the ariaLabel || placeholder
fallback here, otherwise this is a breaking change.
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.
Ah, got it. Didn't think about the breaking nature of this. I'll want some way to not have any aria-label
field, so what do you think about having a ternary like: ariaLabel === undefined ? placeholder : null
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’d do : undefined
, but that looks fine to me!
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.
Yeah, I goofed the example a bit. I used ariaLabel
as the second case in the ternary, it shouldn't have been null
.
@backwardok @ljharb Thanks for the thorough review and good comments! |
This PR would supersede #1590.
Currently the
aria-label
of the date pickerinput
field is being set with the value of theplaceholder
. This seems to be almost always the wrong choice, as anaria-label
is totally different from a placeholder. This PR makes it so you can explicitly set thearia-label
of the input fields and theplaceholder
is no longer used for that attribute.