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

Copy over data attrs from video el to wrapper el. #1321

Merged
merged 2 commits into from Jul 28, 2014

Conversation

Projects
None yet
2 participants
@gkatsev
Member

gkatsev commented Jul 1, 2014

We should be mimicking the video element more closely, so, the wrapper el should not be losing any of the data-attributes that were added before the creation of a player.

Show outdated Hide outdated src/js/player.js
el.id = tag.id;
el.className = tag.className;
attrs = vjs.getAttributeValues(tag);
for (attr in attrs) {

This comment has been minimized.

@heff

heff Jul 1, 2014

Member

You could use vjs.obj.each here. But also, you could move the div creation down to here and do vjs.Component.prototype.createEl.call(this, 'div', attrs).

And there's nothing special about component.createEl, so you could just do vjs.createEl() there if you wanted to simplify that. Component.createEl should actually probably set this.el_ after creating. But don't worry about that for this.

@heff

heff Jul 1, 2014

Member

You could use vjs.obj.each here. But also, you could move the div creation down to here and do vjs.Component.prototype.createEl.call(this, 'div', attrs).

And there's nothing special about component.createEl, so you could just do vjs.createEl() there if you wanted to simplify that. Component.createEl should actually probably set this.el_ after creating. But don't worry about that for this.

This comment has been minimized.

@gkatsev

gkatsev Jul 1, 2014

Member

I'll change to use vjs.obj.each.
As for createEl, that won't work currently because vjs.createEl will copy items over via bracket-notation unless it is an aria- or role attribute which won't pick up things like class attribute or data- attribute which can't be set via the bracket-notation.

@gkatsev

gkatsev Jul 1, 2014

Member

I'll change to use vjs.obj.each.
As for createEl, that won't work currently because vjs.createEl will copy items over via bracket-notation unless it is an aria- or role attribute which won't pick up things like class attribute or data- attribute which can't be set via the bracket-notation.

@heff heff added confirmed labels Jul 1, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 3, 2014

Member

lgtm. Will pull in after the break.

We had some discussion in IRC around improvements to createEl, that don't need to be done for this but are worth documenting.

option 1: a second arg for attributes specifically

vjs.createEl('div', properties, attributes);

option 2: a properties object that's smart enough to read the attributes key

vjs.createEl('div', { attributes: { 'foo': 'bar' } }):
Member

heff commented Jul 3, 2014

lgtm. Will pull in after the break.

We had some discussion in IRC around improvements to createEl, that don't need to be done for this but are worth documenting.

option 1: a second arg for attributes specifically

vjs.createEl('div', properties, attributes);

option 2: a properties object that's smart enough to read the attributes key

vjs.createEl('div', { attributes: { 'foo': 'bar' } }):
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 26, 2014

Member

This is kind of related to #1369. Not sure if we should be considering that.

Member

heff commented Jul 26, 2014

This is kind of related to #1369. Not sure if we should be considering that.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 26, 2014

Member

Definitely related. We need both as well.

Member

gkatsev commented Jul 26, 2014

Definitely related. We need both as well.

el.className = tag.className;
attrs = vjs.getAttributeValues(tag);
vjs.obj.each(attrs, function(attr) {
if (Object.prototype.hasOwnProperty.call(attrs, attr)) {

This comment has been minimized.

@heff
@heff

This comment has been minimized.

@gkatsev

gkatsev Jul 28, 2014

Member

I think I had it here still because I was doing this with a for-loop originally.

@gkatsev

gkatsev Jul 28, 2014

Member

I think I had it here still because I was doing this with a for-loop originally.

@heff heff merged commit b65eb0e into videojs:master Jul 28, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@gkatsev gkatsev deleted the gkatsev:data-attributes branch Aug 6, 2014

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