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

Focus ring now shows up around custom navigation buttons #1563

Merged

Conversation

nkinser
Copy link
Contributor

@nkinser nkinser commented Feb 25, 2019

This PR fixes an issue where when custom navigation buttons are provided, the focus ring for keyboard-only users did not appear around the custom buttons. This issue is fixed by only supplying a tabindex when the default navigation buttons are used. This means that in order for custom navigation buttons to receive keyboard focus a tabindex must be specified on the object passed in.

Before

calendar_tabbing_before

After

calendar_tabbing_after

@coveralls
Copy link

coveralls commented Feb 25, 2019

Coverage Status

Coverage increased (+0.03%) to 84.529% when pulling a84e51a on nkinser:nk--fix-custom-nav-buttons-focus-ring into dd1ab72 on airbnb:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Seems good; worth a regression test?

.gitignore Show resolved Hide resolved
src/components/DayPickerNavigation.jsx Outdated Show resolved Hide resolved
@nkinser
Copy link
Contributor Author

nkinser commented Feb 26, 2019

@ljharb How would I add regression tests for this?
I do think this could be considered a breaking change if anyone supplies custom navigation buttons, this new change will cause the buttons to be skipped over with keyboard navigation.

@ljharb
Copy link
Member

ljharb commented Feb 26, 2019

Hmm, that's a fair point.

As for a test, I'd assume we could provide one test that uses default buttons, and asserts the tabIndex is present, and another that uses custom ones, and asserts that it's not?

Even better would be adding those test cases to master first, and then updating them in this PR, which would make the breakage very clear.

@nkinser
Copy link
Contributor Author

nkinser commented Feb 26, 2019

@ljharb Would it be possible to check in DayPickerNavigation if the supplied custom buttons have a tabIndex and then only omit the tabIndex prop if the custom buttons have it? That way it wouldn't be a breaking change

@ljharb
Copy link
Member

ljharb commented Feb 26, 2019

Not reliably, no - all we can do is render what they give us.

@nkinser nkinser force-pushed the nk--fix-custom-nav-buttons-focus-ring branch from 0c0b0e7 to a84e51a Compare February 27, 2019 07:00
@nkinser
Copy link
Contributor Author

nkinser commented Feb 27, 2019

@ljharb do the test changes look good to you?

Copy link
Member

@ljharb ljharb 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, thanks!

@ljharb ljharb added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Feb 27, 2019
@nkinser nkinser merged commit ec05ee2 into react-dates:master Feb 27, 2019
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.

3 participants