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: localize aria-labels #4027

Merged
merged 12 commits into from
Feb 8, 2017
Merged

fix: localize aria-labels #4027

merged 12 commits into from
Feb 8, 2017

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Feb 2, 2017

Fix #2728

@@ -26,7 +26,7 @@ class CaptionsButton extends TextTrackButton {
*/
constructor(player, options, ready) {
super(player, options, ready);
this.el_.setAttribute('aria-label', 'Captions Menu');
this.el_.setAttribute('aria-label', this.localize('Captions Menu'));
Copy link
Member Author

Choose a reason for hiding this comment

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

@OwenEdwards thoughts on whether we need to create a new item for this or if we can just re-use Captions and Chapters etc for these button labels?

Copy link
Member

Choose a reason for hiding this comment

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

@gkatsev see #4034 - that PR removes this aria-label attribute in all menus.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. I'll rebase this after that one gets merged then.

@gkatsev gkatsev requested a review from OwenEdwards February 2, 2017 20:15
@gkatsev gkatsev added the a11y This item might affect the accessibility of the player label Feb 2, 2017
Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

One small comment/question, but otherwise LGTM.

lang/en.json Outdated
@@ -1,4 +1,6 @@
{
"audio player": "audio player",
"video player": "video player",
Copy link
Member

Choose a reason for hiding this comment

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

Should these be title-cased for consistency as long as we're pushing toward a major release?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, these and others (e.g. "progress bar") ought to be title-cased.

@gkatsev gkatsev force-pushed the aria-label-locaalization branch from 181c09f to 8d869b4 Compare February 8, 2017 06:35
Copy link
Member

@OwenEdwards OwenEdwards left a comment

Choose a reason for hiding this comment

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

One minor capitalization change needed.

@@ -48,7 +48,7 @@ class SeekBar extends Slider {
return super.createEl('div', {
className: 'vjs-progress-holder'
}, {
'aria-label': 'progress bar'
'aria-label': this.localize('progress bar')
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to be in Title Case too?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops

Copy link
Member

@OwenEdwards OwenEdwards left a comment

Choose a reason for hiding this comment

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

LGTM

@gkatsev gkatsev merged commit 0ac1269 into master Feb 8, 2017
@gkatsev gkatsev deleted the aria-label-locaalization branch February 8, 2017 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants