Respect controls: false, allows to create player without controls #65
Conversation
komachi
force-pushed
the
disable-controls
branch
from
2244dcc to
027063d
Compare
| @@ -67,7 +67,7 @@ const createIframeEmbed = (params) => { | |||
| * The DOM element that will ultimately be passed to the `bc()` function. | |||
| */ | |||
| const createInPageEmbed = (params) => { | |||
| const {embedOptions} = params; | |||
| const {embedOptions, options} = params; | |||
There was a problem hiding this comment.
I don't think we would want to add a new options property when we already have a embedOptions property that serves the same purpose.
There was a problem hiding this comment.
I don't think this should be moved to embedOptions. controls: Boolean is video.js option, original issue appears because player-loader create controls attribute without respecting passed options, and attribute takes precedence over options object. It's already noted in README that options object will be passed to video.js.
There was a problem hiding this comment.
I think this is an issue with the Brightcove Player's expectation of controls and how Video.js treats controls as well.
In video.js the option takes precedent over the attribute. However, The BCP sets the controls option based on the presence of the controls attribute.
In this case, I think that this really should be an embedOption because we're changing how the embed should look. It's separate from what the option does. We should also then take a look at whether the behavior in the Brightcove Player is ideal here (and also make sure that we don't break things if we make any changes)
There was a problem hiding this comment.
Sorry, I misunderstood the original issue. Note to self: don't look at PRs first thing in the morning! 😄
That said, I don't think we want to introduce a new options object when we could simply define a new property of embedOptions.
Do you think you could also make sure it's documented in README.md?
Thanks so much for the PR!
|
Until this is merged, you as a hack you can force-hide controls with css. |
Looking at why there is controls when I pass option to disable controls, I found that you add controls attr without respecting named option. This small PR fixes this and adds regression test.
This should close brightcove/react-player-loader#60