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

Refactor MuteToggle#update #4058

Merged

Conversation

kevinlitchfield
Copy link
Contributor

Since MuteToggle#update performs two distinct tasks, I thought it might be worth doing a small refactoring, if the maintainers are in agreement.

Summary of proposed changes:

  • Extract updateIcon_ and updateControlText_ private methods.
  • Expand original comment for update and comment updateIcon_ and updateControlText_.
  • Rename toMute to text. toMute sounds to me like a Boolean, but its role is to store the correct current text for controlText.
  • Add missing JSDoc directive to update.

Question

I'm unsure of the meaning of the following comment on the section of code that I've put in updateControlText_:

  // This causes unnecessary and confusing information for screen reader users.

Does this mean:

  1. There is an existing problem with this code for users of screen readers?
  2. There is not a problem, but there would be a problem if MuteToggle.controlText were updated each time volume was updated?

I've left that part out for now pending an answer.

@gkatsev
Copy link
Member

gkatsev commented Feb 13, 2017

This looks great, thanks @kevinlitchfield.

I think the punctuation of that comment is misleading. I believe it means that the check is necessary so that the label won't get changed if there's no need to change it. If the label gets changes each time (even if to the same text), the screen reader would read it out and could cause confusion. So, #.2 is the correct meaning, I believe.

@brandonocasey
Copy link
Contributor

@kevinlitchfield This needs a rebase on master when you get a chance

@kevinlitchfield
Copy link
Contributor Author

@brandonocasey 👍

@brandonocasey brandonocasey merged commit a04f387 into videojs:master Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants