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.

Sign up for GitHub

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

Respect controls: false, allows to create player without controls #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Respect controls: false, allows to create player without controls #65

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/create-embed.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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)


// We DO NOT include the data-account, data-player, or data-embed attributes
// here because we will be manually initializing the player.
Expand Down Expand Up @@ -107,7 +107,9 @@ const createInPageEmbed = (params) => {
el.setAttribute(paramsToAttrs[key], value);
});

el.setAttribute('controls', 'controls');
if (!options || options.controls !== false) {
el.setAttribute('controls', 'controls');
}
el.classList.add('video-js');

return el;
Expand Down
12 changes: 12 additions & 0 deletions test/create-embed.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,18 @@ QUnit.module('create-embed', function(hooks) {
assert.notOk(embed.hasAttribute('data-embed'), 'we never include data-embed because we want to init players ourselves');
});

QUnit.test('creates player without controls', function(assert) {
const embed = createEmbed({
refNode: this.fixture,
refNodeInsert: 'append',
options: {
controls: false
}
});

assert.notOk(embed.hasAttribute('controls'), 'doesn\'t have controls attribute');
});

QUnit.test('populates certain attributes from params', function(assert) {
const embed = createEmbed({
refNode: this.fixture,
Expand Down