Skip to content
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

Use aria-label instead of visually hidden label #280

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

wyattdanger
Copy link
Collaborator

Instead of using a label element with CSS to visually hide it, use the
aria-label property on the input element itself. Having hidden text for
screen readers could cause the label value to be read aloud twice by the screen
reader: first when navigating nodes in the form, and then again when focusing
the input.

Instead of using a `label` element with CSS to visually hide it, use the
`aria-label` property on the `input` element itself. Having hidden text for
screen readers could cause the label value to be read aloud twice by the screen
reader: first when navigating nodes in the form, and then again when focusing
the input.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.141% when pulling 2401cdb on swb--hidden-label-to-aria-label into 0bd7072 on master.

<input
aria-label={placeholder}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that this prop defaults to the empty string, so i think you might want {placeholder || null} here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placeholder defaults to 'Select Date'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, my eye looked at the wrong line

@ljharb ljharb added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Feb 1, 2017
@ljharb
Copy link
Member

ljharb commented Feb 1, 2017

Technically I think this is a breaking change, because somebody might have been using the CSS class to style it.

@wyattdanger
Copy link
Collaborator Author

@ljharb I wouldn't expect anybody to use .DateInput__label but I suppose better safe than sorry?

@majapw
Copy link
Collaborator

majapw commented Feb 1, 2017

This looks reasonable to me. I agree with @ljharb that there is definitely a possibility that someone was overriding the class to create a visible label.

.... In other news, can we just make a visible label and ship that with an option to hide it for our own designs when we need to? I think that would be nice.

@majapw majapw merged commit fccf6b3 into master Feb 1, 2017
@majapw majapw deleted the swb--hidden-label-to-aria-label branch February 1, 2017 19:31
@backwardok
Copy link
Contributor

@majapw or even better, we make a visible label and you can't hide it and it just always shows up for everyone!

@majapw
Copy link
Collaborator

majapw commented Feb 1, 2017

I suppose the design on airbnb.com now has an actual visible label so we could ostensibly sneak that in. I think that mobile designs will not be so friendly though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants