-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Adds support to set "mute" attribute by default. #183
Conversation
Adds examples to set mute by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After reviewing this closely, I want to accept this PR. Testing right now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great overall! Couple minor issues, and then we can merge :)
src/api/player.js
Outdated
@@ -47,6 +47,12 @@ function Player(selector, contentInfo) { | |||
// Expose a public .isPaused attribute. | |||
this.isPaused = false; | |||
|
|||
// Expose a plublic .isMuted attribute. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"public"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Player.prototype.mute = function(muteState) { | ||
var data = { | ||
muteState: muteState | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should toggle this.isMuted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm already toggling in:
205 case 'muted':
206 this.isMuted = data;
207 this.emit('mute', data);
208 break;
I followed the play/pause example. Is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry, you are right. I will probably need to change my volume impl to look more like what you did. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
examples/video/index.js
Outdated
@@ -23,7 +23,8 @@ function onLoad() { | |||
height: 480, | |||
video: 'congo_2048.mp4', | |||
is_stereo: true, | |||
loop: false, | |||
loop: true, | |||
//muted: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you uncomment this and set muted: true, then the video does start muted but the UI doesn't show the muted state. Can this be fixed easily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* Makes the video example start muted by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last change!
examples/video/index.js
Outdated
@@ -23,7 +23,8 @@ function onLoad() { | |||
height: 480, | |||
video: 'congo_2048.mp4', | |||
is_stereo: true, | |||
loop: false, | |||
loop: true, | |||
muted: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not check this in as true :). Maybe check it in commented out as true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to clarify: I think we should leave loop: false and comment out muted: true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me.
* Comment out the muted attribute.
Today there is no way to mute videos by default.
To do that a workaround is needed to set the volume to "0" on the "ready" event. It causes some unwanted noises if the browser performance is not good.
If this pull request is accepted, developers can set the video to be muted by default.
Besides, I changed the volume icon to mute/unmute the video and added a volume range on the user interface to control the volume level.
This feature will be very useful to me in particular and hope this can help others too.