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

Keyboard nav and aria improvements #132

Merged
merged 5 commits into from
Mar 8, 2016
Merged

Keyboard nav and aria improvements #132

merged 5 commits into from
Mar 8, 2016

Conversation

limscoder
Copy link
Contributor

What does this PR do?

keyboard nav:

  • navigate between weeks with up/down arrow keys when inner grid is focused
  • navigate between years with up/down arrow keys when outer div is focused

aria:

  • added additional aria labels so screen reader reads date when focus changes (tested on OSX/chrome/voiceover and win8/IE11/JAWS)

Any background context you want to provide?

There are 5 commits. Started with some refactoring to simplify DayPicker, then added keyboard nav features and aria labels to date cells, so that it works better with screen readers.

Where should the reviewer start?

Look at each commit independently.

How should this be manually tested?

Test keyboard nav in example app.
Test with screen reader in example app (cmd-f5 to enable voiceover in OSX).

@gpbl
Copy link
Owner

gpbl commented Feb 20, 2016

Hi @limscoder thanks so much for this PR! Really appreciated. I'm on vacation now, I can give a look to it in the next days. 🌴

@limscoder
Copy link
Contributor Author

Any progress on reviewing this? Can I answer any questions?

@@ -429,10 +468,17 @@ export default class DayPicker extends Component {
tabIndex = this.props.tabIndex;
}
}

const ariaLabel = this.props.localeUtils.formatDate ?
this.props.localeUtils.formatDate(day) : day.toDateString();
Copy link
Owner

Choose a reason for hiding this comment

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

The formatDate here is a new function to add to the LocaleUtils right? I can take care of this (need to update docs, tests, etc. 😄)
We should add the locale props as second argument to the function, so implementers can use their own localization utils.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, new function.

@gpbl gpbl modified the milestone: v2 Mar 8, 2016
@gpbl gpbl merged commit 8cb34b9 into gpbl:master Mar 8, 2016
@gpbl gpbl mentioned this pull request Mar 8, 2016
@gpbl
Copy link
Owner

gpbl commented Mar 8, 2016

Thanks again @limscoder, those are great additions. They are going in the next major release, I'm working on it this week.

@limscoder
Copy link
Contributor Author

thx

gpbl added a commit that referenced this pull request Apr 8, 2016
This is required to add the aria-label attribute. See #132 for more details.
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.

2 participants