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

Add keyboard accessibility #757

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vahissan
Copy link
Contributor

@vahissan vahissan commented Dec 1, 2020

Description

  • Added ability to navigate the calendar using the TAB key
  • Added ability to close the calendar by pressing the ESC key when focused on the calendar or the input field
  • Added ability to open the calendar by pressing the ArrowDown key (useful when it was closed by ESC key)

Motivation and Context

Fixes #415 (at least partially)
I was using the library and would like to have the ability to operate the date picker without a mouse. I'm sure many others would like it too.

Checklist

[ x ] I have not included any built dist files (us maintainers do that prior to a new release)
[ x ] I have added tests covering my changes
[ x ] All new and existing tests pass
[ ] My changes required the documentation to be updated
  [ ] I have updated the documentation accordingly
  [ ] I have updated the TypeScript 1.8 type definitions accordingly
  [ ] I have updated the TypeScript 2.0+ type definitions accordingly

@vahissan
Copy link
Contributor Author

vahissan commented Dec 1, 2020

These are the things from the original issue which I didn't add in this PR:

  • aria-label attributes for the td cells: I don't think it's required because the screen readers pickup the text inside the td elements without issue.
  • arrow key navigation within the calendar: I think it would impact the performance. Also, I wanted to add the basic functionality first, and maybe improve it later.

@vahissan
Copy link
Contributor Author

vahissan commented Dec 1, 2020

CI build failed due to these reasons:

  1. There's a peer dependency check failing on npm install due to npm v7's breaking changes.
  2. Unit tests failed because they depend on the built dist files.

@arqex
Copy link
Owner

arqex commented Dec 1, 2020

Hey @vahissan !

Thanks for this great PR, I need to find some time to review it :)

If you know what's failing with the v7 of npm, can create a different PR with the fix and we merge it before this one?

@vahissan
Copy link
Contributor Author

vahissan commented Dec 1, 2020

Thanks @arqex !

Sure, I'll take a look. Adding --legacy-peer-deps to npm install in CI is a workaround if you're interested.

EDIT: created PR #758

@vahissan
Copy link
Contributor Author

vahissan commented Dec 4, 2020

CI build failed due to these reasons:

  1. There's a peer dependency check failing on npm install due to npm v7's breaking changes.
  2. Unit tests failed because they depend on the built dist files.

Reason 1 is resolved. But CI still fails due to reason 2. Looks like CI does not create a new build before running unit tests.

@vahissan
Copy link
Contributor Author

Hey @arqex !

I know you must be busy. Just wanted to send a reminder in case you forgot.

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.

Keyboard accessibility
2 participants