Copy over data attrs from video el to wrapper el. #1321
Conversation
| el.id = tag.id; | ||
| el.className = tag.className; | ||
| attrs = vjs.getAttributeValues(tag); | ||
| for (attr in attrs) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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 option 2: a properties object that's smart enough to read the attributes key |
|
This is kind of related to #1369. Not sure if we should be considering that. |
|
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)) { |
There was a problem hiding this comment.
obj.each already checks hasOwnProperty
https://github.com/videojs/video.js/blob/v4.6.0/src/js/lib.js#L85
There was a problem hiding this comment.
I think I had it here still because I was doing this with a for-loop originally.
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.