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: accessibility bugs with the VolumeBar #4023

Merged
merged 1 commit into from
Feb 2, 2017

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Feb 2, 2017

Description

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
  • Reviewed by Two Core Contributors

Copy link
Member

@gkatsev gkatsev left a comment

Choose a reason for hiding this comment

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

Also, when merging, I think github won't autoclose the issues if they're listen that way. I think it looks specifically for fixes #x. However, I do like the listing, so, maybe something like:

Fixes #123, #234
* #123: foo bar
* #234 bar baz

@@ -27,7 +27,7 @@ class VolumeBar extends Slider {
super(player, options);
this.on('slideractive', this.updateLastVolume_);
this.on(player, 'volumechange', this.updateARIAAttributes);
player.ready(() => this.updateARIAAttributes);
player.ready(() => this.updateARIAAttributes());
Copy link
Member

Choose a reason for hiding this comment

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

heh

@@ -106,7 +106,7 @@ class VolumeBar extends Slider {
*/
updateARIAAttributes(event) {
// Current value of volume bar as a percentage
const volume = (this.player_.volume() * 100).toFixed(2);
const volume = Math.round((this.player_.volume() * 100).toFixed(2));
Copy link
Member

Choose a reason for hiding this comment

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

no need to the .toFixed(), that's what adds the two decimal places. Maybe just a .toString()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to toString()

@brandonocasey brandonocasey force-pushed the fix/volume-bar-aria-attributes branch from 656701e to 98863b8 Compare February 2, 2017 17:02
@gkatsev gkatsev added the a11y This item might affect the accessibility of the player label Feb 2, 2017
@brandonocasey brandonocasey force-pushed the fix/volume-bar-aria-attributes branch from 98863b8 to cb9bff7 Compare February 2, 2017 17:03
@brandonocasey
Copy link
Contributor Author

I wasn't even aware that GitHub could auto-close issues. I will have to use the suggested format more often.

@@ -106,7 +106,7 @@ class VolumeBar extends Slider {
*/
updateARIAAttributes(event) {
// Current value of volume bar as a percentage
const volume = (this.player_.volume() * 100).toFixed(2);
const volume = (this.player_.volume() * 100).toString();
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd still want Math.round or Math.floor to make sure we are getting an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@brandonocasey brandonocasey force-pushed the fix/volume-bar-aria-attributes branch from cb9bff7 to e7d62e6 Compare February 2, 2017 17:47
* Fixes #4021: drop the decimal places on aria-valuenow and aria-valuetext
* Fixes #4022: aria-valuenow and aria-valuetext not set initially
@brandonocasey brandonocasey force-pushed the fix/volume-bar-aria-attributes branch from e7d62e6 to 3a317fa Compare February 2, 2017 17:49
@brandonocasey brandonocasey merged commit da2a1e0 into master Feb 2, 2017
@brandonocasey brandonocasey deleted the fix/volume-bar-aria-attributes branch February 2, 2017 21:10
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
2 participants