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.

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

Options in data-setup attribute are ignored when the initialization is called before autoSetup #1514

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@chikathreesix
Contributor

chikathreesix commented Sep 17, 2014

Because only vjs.autoSetup picks the option from data-setup, this attribute will be ignored if the initialization is called before that. Sometimes I face this issue and options in data-setup are ignored, so I made Player.getTagSetting pick the option from data-setup as well.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 17, 2014

Member

Interesting, thanks! I don't think we've run into this before because most people either use autoSetup by itself,

<video data-setup="[options]"></video>

or they don't use the data-setup attribute and pass in the options in the init function

<video> </video>
videojs(id, options)

But I could see there potentially being a situation where you have two async scripts, the autoSetup scripts and another script that references the player, and that would cause this issue. Is that the situation you have?

Member

heff commented Sep 17, 2014

Interesting, thanks! I don't think we've run into this before because most people either use autoSetup by itself,

<video data-setup="[options]"></video>

or they don't use the data-setup attribute and pass in the options in the init function

<video> </video>
videojs(id, options)

But I could see there potentially being a situation where you have two async scripts, the autoSetup scripts and another script that references the player, and that would cause this issue. Is that the situation you have?

};
tagOptions = vjs.getElementAttributes(tag);
dataSetup = tagOptions['data-setup'];

This comment has been minimized.

@heff

heff Sep 17, 2014

Member

If we're doing this here now we should also remove it from the autoSetup script. And move the code comments over from there over to this one. Just the "If empty string, make it a parsable json object" one.

options = vjs.JSON.parse(options || '{}');

@heff

heff Sep 17, 2014

Member

If we're doing this here now we should also remove it from the autoSetup script. And move the code comments over from there over to this one. Just the "If empty string, make it a parsable json object" one.

options = vjs.JSON.parse(options || '{}');

@heff

View changes

Show outdated Hide outdated test/unit/setup.js
@@ -1 +1,19 @@
module('Setup');
test('should set options from data-setup even if autoSetup is not called before initialisation', function(){
var fixture = document.getElementById('qunit-fixture');

This comment has been minimized.

@heff

heff Sep 17, 2014

Member

We should use our MakePlayer test helper here. It prevents executing the full player stack and can help avoid unrelated errors from happening.

makePlayer: function(playerOptions, videoTag){

You could do something like:

var el = PlayerTest.makeTag();
el.setAttribute('data-setup', '...');
var player = PlayerTest.makePlayer({}, el);
@heff

heff Sep 17, 2014

Member

We should use our MakePlayer test helper here. It prevents executing the full player stack and can help avoid unrelated errors from happening.

makePlayer: function(playerOptions, videoTag){

You could do something like:

var el = PlayerTest.makeTag();
el.setAttribute('data-setup', '...');
var player = PlayerTest.makePlayer({}, el);
@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 18, 2014

Contributor

Thank you for your quick response! Yes, that's what exactly my situation is. Because a few of my scripts refer the Player instance, not sure who to be the first. Therefore I am having the option in data-setup.
By the way, I've fixed my code according to your comments.

Contributor

chikathreesix commented Sep 18, 2014

Thank you for your quick response! Yes, that's what exactly my situation is. Because a few of my scripts refer the Player instance, not sure who to be the first. Therefore I am having the option in data-setup.
By the way, I've fixed my code according to your comments.

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc Sep 18, 2014

Member

Thanks! This looks good to me, so we should pull this into the next release.

Member

mmcc commented Sep 18, 2014

Thanks! This looks good to me, so we should pull this into the next release.

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 19, 2014

Contributor

Thanks!

Contributor

chikathreesix commented Sep 19, 2014

Thanks!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

Good to go for this week's release.

Member

heff commented Sep 29, 2014

Good to go for this week's release.

@heff heff closed this in f47bf26 Sep 29, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Sep 29, 2014

Member

Merged in. Thank you!

Member

heff commented Sep 29, 2014

Merged in. Thank you!

@chikathreesix

This comment has been minimized.

Show comment
Hide comment
@chikathreesix

chikathreesix Sep 30, 2014

Contributor

Thank you!

Contributor

chikathreesix commented Sep 30, 2014

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment