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.

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

Move player id generation #845

Closed
wants to merge 1 commit into from
Closed

Conversation

dmlap
Copy link
Member

@dmlap dmlap commented Nov 25, 2013

If the video element has a id attribute when video.js was created, it used that as the player id. If the video did not have an id, however, the player would get an auto-generated component id that started with "undefined_..." and the player element received a id like "vjs_video_...". This change modifies the player init order so that the video element is given an autogenerated id early in the process, which is then picked up by the player component when it gets around to initialization. This unifies the player component id and tag id and is more consistent with the structure created when a video element with a user-defined id is passed in.

If the video element has a id attribute when video.js was created, it used that as the player id. If the video did not have an id, however, the player would get an auto-generated component id that started with "undefined_..." and the player element received a id like "vjs_video_...". This change modifies the player init order so that the video element is given an autogenerated id early in the process, which is then picked up by the player component when it gets around to initialization. This unifies the player component id and tag id and is more consistent with the structure created when a video element with a user-defined id is passed in.
@heff
Copy link
Member

heff commented Nov 25, 2013

Awesome. Just noticed this today too.

@gkatsev
Copy link
Member

gkatsev commented Nov 25, 2013

This is one of the things I mentioned in #844.

@dmlap
Copy link
Member Author

dmlap commented Nov 27, 2013

There are some interesting points raised in #844 but I think this is a good change for the moment, independent of whether ids are problematic in the grand scheme of things.

@heff heff closed this in 1cab783 Dec 2, 2013
@dmlap dmlap deleted the hotfix/missing-ids branch December 3, 2013 00:02
@albell
Copy link
Contributor

albell commented Mar 11, 2014

Good for semantic parallelism, but I'm uneasy that the code seems to be hardening around the use of IDs. I used to be skeptical of the whole puritanlcal "classes are for CSS hooks, data-* attributes are for JS hooks" philosophy, but after being burned a few times I am now convinced that IDs should basically be banished going forward whenever possible, except for a few uses like on-page anchors. It's a win for maintenance, readability, and reusability. It's fine to have an element that isn't reused on the page, but it's quite another to say that it will never-ever-ever recur in any foreseeable (or unforeseeable) future page tweak. querySelectorAll doesn't work on oldIE, but depending on when support is scheduled to drop for oldIE it could be polyfilled (internally?). This isn't a huge architectural change really, just switching identifiers out of the ID space. Are IE 6 & 7 still officially supported?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants