Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

adjust volume ranges so muted(true) and vol=0 do not use the same icons #4425

Merged
merged 2 commits into from
Jul 26, 2017

Conversation

marguinbc
Copy link
Contributor

Description

Potential fix for #4424

Specific Changes proposed

Change the volume levels used to select the videojs-icon-volume-* classes so muted(true) and volume(0) do not use the same icon. Fixes an issue where sliding the volume slider to 0 has the appearance of setting muted(true) but toggling the mute-toggle/volumeMenuButton does not restore the volume. Only setting the volume using the slider removes the muted icon appearance.

Requirements Checklist

  • [ x ] 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)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gkatsev gkatsev added the 5.x label Jun 26, 2017
@@ -81,11 +81,11 @@ class MuteToggle extends Button {
const vol = this.player_.volume();
let level = 3;

if (vol === 0 || this.player_.muted()) {
if (this.player_.muted()) {
Copy link
Member

Choose a reason for hiding this comment

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

what about only making this change?
I think level 1 is supposed to imply that the volume is very low and not just 0. So, if muted, level 0, if <0.33, level 1, if less than .67 level 2, else level 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That works for me. Only reason I changed the other levels is because I thought no ring seems like a good indicator for 0 sound, 1 ring for up to 50% and two rings for over 50%

@mister-ben
Copy link
Contributor

I'm in two minds about this. It does make sense to have some UI change when muting a player with zero volume so it doesn't appear that nothing has happened, but it might cause confusion if a user thinks the slider isn't getting all the way to zero when they drag it to the left. It does illustrate that the v6 behaviour is a big improvement in UX.

@gkatsev
Copy link
Member

gkatsev commented Jul 19, 2017

The fact that volume 0 and muted are separate is kind of weird from a UI perspective. Kind of makes sense from an implementation perspective, though. @marguinbc made a PR to make the swf behave like the video element does wrt muted and volume (videojs/video-js-swf#230).
@mister-ben do you think we should not have this change? Unfortunately, I don't think we can really make any other changes to improve this in v5 because it may be a breaking change.

@mister-ben
Copy link
Contributor

I'd go with this on balance. I've been following the work on the swf, great improvement.

Copy link
Contributor

@brandonocasey brandonocasey left a comment

Choose a reason for hiding this comment

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

PR LGTM, but I am glad we have a better solution in videojs 6

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants