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

Fix incorrect VO for selected check-in date #1451

Conversation

nkinser
Copy link
Contributor

@nkinser nkinser commented Nov 5, 2018

This PR fixes an accessibility bug where VO incorrectly defines a selected check-in date in calendar as unavailable. This is fixed by checking if the day is both blocked and selected, and if this is true the phrase should be dateIsSelected.

@coveralls
Copy link

coveralls commented Nov 5, 2018

Coverage Status

Coverage remained the same at 85.049% when pulling b0e86b7 on nkinser:nk-fix-incorrect-VO-for-selected-checkin-date into 5bf3fa9 on airbnb:master.

@nkinser
Copy link
Contributor Author

nkinser commented Nov 5, 2018

@majapw Can you take a look at this? Thanks!

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

I think the conditional can be simplified a bit. :)

if (modifiers.has(BLOCKED_MODIFIER)) {
if (modifiers.has(BLOCKED_MODIFIER) && selected) {
ariaLabel = getPhrase(dateIsSelected, formattedDate);
} else if (modifiers.has(BLOCKED_MODIFIER)) {
ariaLabel = getPhrase(dateIsUnavailable, formattedDate);
} else if (selected) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

would it make sense to just move the if(selected) condition to be fire? e.g.

if (selected) {
    ariaLabel = getPhrase(dateIsSelected, formattedDate);
  } else if (modifiers.has(BLOCKED_MODIFIER)) {
    ariaLabel = getPhrase(dateIsUnavailable, formattedDate);
  } else ...

@nkinser nkinser force-pushed the nk-fix-incorrect-VO-for-selected-checkin-date branch from f362968 to b0e86b7 Compare November 7, 2018 07:13
@nkinser
Copy link
Contributor Author

nkinser commented Nov 7, 2018

@majapw PTAL! Thanks!

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

Looks great!

@majapw majapw merged commit eafc6f1 into react-dates:master Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants