Respect muted attribute #2408
Respect muted attribute #2408
Conversation
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0792defed1b781c6443f56afc5c21bc7966e8658 (Please note that this is a fully automated comment.) |
|
@pam retry |
|
Volume zero isn't the same as muted. I'm not sure this is quite right. When you unmute it should go back to the previously set volume. The controls should be checking if the player is muted, not only rely on volume zero. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 0792defed1b781c6443f56afc5c21bc7966e8658 (Please note that this is a fully automated comment.) |
|
Changing all the things to use dotnotation is great. Though, it would be nice if you can leave a comment in the PR view where the meat of the changes are so it'll be easier to see it. |
|
@heff I don't think I follow. This is the muted attribute on initial play and we don't have any notion of default or persisted volume across players (anymore), so at the point we're setting volume to 0 the current volume is going to be 100% unless someone sets the volume on the I agree that this changes if we decide to reimplement default / persisted volume down the road, but as it stands now, I'm struggling to think of an edge case where this fails given the current setup. @gkatsev Yeah, definitely. The original description message includes a link to the commit with the actual changes, but I don't think it's noticeable enough (everyone just immediately clicks "files" anyway). Notedd |
|
Given that video begin with volume at 1, the |
|
Ah wow, you're totally right. I was going based off the mute icon, but it also goes to that on volume 0, not just on mute. Clicking to unmute is totally busted with this approach. You guys win. This time. |
|
Well...definitely should have used |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 93320dc7b3f8d7e6ab49fc3a7fc50a7da85f27aa (Please note that this is a fully automated comment.) |
|
@heff @gkatsev bump for thoughts on this approach: 93320dc Also considered just going with something like |
|
Cool. Is this enough to fix the issue? I think we need to get consistent with how we handle these attributes. One way or the other. eg controls |
|
Yes, it fixes the issue outlined in #2358, and I'm struggling to think of edge cases with this one, but your reticence has me second guessing. The logic required to get controls to work is much more of a rats nest, so in terms of complexity this doesn't need to come close. That being said, I agree on the consistency note. I'll move this closer to controls. |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d26cc5eb01c2f1d13ed5e7e042c3d48617f4d9da (Please note that this is a fully automated comment.) |
|
I just assumed the volume controls would need to be updated to check the Sent from mobile |
use `#muted` instead of `#volume` stylistically move closer to existing settings
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: bed80eb (Please note that this is a fully automated comment.) |
|
Something pulled into master is causing these tests to break. Looking into it. |
|
The |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 318b3cd (Please note that this is a fully automated comment.) |
|
@pam retry and please, for the love, work this time. edit: Pam doesn't like begging apparently. |
|
@pam retry |
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 318b3cd (Please note that this is a fully automated comment.) |
|
LGTM |
| @@ -503,7 +506,7 @@ class Player extends Component { | |||
| 'autoplay': this.options_.autoplay, | |||
| 'preload': this.options_.preload, | |||
| 'loop': this.options_.loop, | |||
| 'muted': this.options_.muted, | |||
| 'muted': this.muted_, | |||
There was a problem hiding this comment.
Let's change this back to options.muted and remove the muted_ setting above. In the future we need to handle properties like this better. Decide where the state should live and how to pass that between techs.
|
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: 73ebb7b (Please note that this is a fully automated comment.) |
The
mutedfix is 88634b0. The rest of the changes are switching the module over to dot notation.My personal opinion is that we should just do this in every file we touch from now on. If you all disagree, I'll revert 0792def.