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

Added an 'attributes' arg to createEl() #2589

Closed
wants to merge 5 commits into from
Closed

Conversation

heff
Copy link
Member

@heff heff commented Sep 15, 2015

fixes #2176

Properties and attributes need to be handled separately in createEl(). This adds a third arg for attributes and a deprecation warning for the cases were we expected attributes to come through the properties object.

// We originally were accepting both properties and attributes in the
// same object, but that doesn't work so well.
if (propName.indexOf('aria-') !== -1 || propName === 'role' || propName === 'type') {
log.warn(`Setting attributes in the second argument of createEl()
Copy link
Member

Choose a reason for hiding this comment

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

does this have a bunch of extra whitespace in it? Might want to grab https://www.npmjs.com/package/tsml

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool. Pulled in tsml.

@gkatsev
Copy link
Member

gkatsev commented Sep 15, 2015

LGTM, we probably want to mention this in the wiki for the 5.0 transition.

@heff
Copy link
Member Author

heff commented Sep 15, 2015

Yeah, good point. I'll add it.

On Mon, Sep 14, 2015 at 8:38 PM, Gary Katsevman [email protected]
wrote:

LGTM, we probably want to mention this in the wiki for the 5.0 transition.


Reply to this email directly or view it on GitHub
#2589 (comment).

@heff heff closed this in f10d8d0 Sep 15, 2015
@@ -29,25 +31,28 @@ export function getEl(id){
* @return {Element}
* @function createEl
*/
export function createEl(tagName='div', properties={}){
export function createEl(tagName='div', properties={}, attributes={}){
Copy link
Member

Choose a reason for hiding this comment

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

the jsdocs for this were never added.
Also, we should probably document exactly what the different between the two would be.

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.

vjs.obj.merge and vjs.createEl don't handle HTML attributes (which are case-insensitive) correctly
2 participants