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

Ignore whitelisted attributes for native custom elements. #3752

Merged
merged 1 commit into from
Jun 10, 2015

Conversation

jhicken
Copy link
Contributor

@jhicken jhicken commented Apr 27, 2015

#140

This will cause react to ignore the regular props/attributes whitelist if the tag name for an element contains a - or if the element has an is attribute applied to it. This makes you able to use native custom elements via the is attribute or by adding a - your custom element tag name.

I just threw this together real quick. I just want to make sure I'm running down a good path with it.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zpao
Copy link
Member

zpao commented Apr 27, 2015

This has a ton of overlap with #3067, we should probably focus our efforts. cc @JSFB to figure out the best path forward.

@jimfb
Copy link
Contributor

jimfb commented Apr 27, 2015

Yeah, let's check the status of #3067 and once we figure out what to do there, we can take another look at this PR.

@jhicken
Copy link
Contributor Author

jhicken commented Apr 28, 2015

sounds good

@jhicken
Copy link
Contributor Author

jhicken commented Jun 9, 2015

So @jimfb 's commit #3067 took care of most of this. We still have one last problem to resolve. When there is an "is" attribute we should be ignoring the whitelist for other attributes on that element. I updated my PR It should take care of this last little problem. @zpao can we look at this again?

@jimfb
Copy link
Contributor

jimfb commented Jun 9, 2015

Cool, this looks good to me. 👍

cc @zpao @spicyj Any objections or is this one good to go? (modulo travis build failure)

@sophiebits
Copy link
Collaborator

Should be props.is != null. Can we extract the logic to a helper function? (Side note: when is this._tag null?)

Ideally we would do something intelligent when the value of the is attribute changes since I assume we need to remount the element to have things work properly. (That sounds harder though and certainly doesn't need to be done in this PR.)

@@ -686,7 +686,7 @@ ReactDOMComponent.Mixin = {
} else if (lastProp) {
deleteListener(this._rootNodeID, propKey);
}
} else if (this._tag.indexOf('-') >= 0) {
} else if (this._tag.indexOf('-') >= 0 || props.hasOwnProperty('is')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use nextProps here, not props (which is undefined).

@jimfb
Copy link
Contributor

jimfb commented Jun 9, 2015

@spicyj Just to confirm, you're looking for a helper function that takes in tag&props to decide if the element is a webcomponent?

@sophiebits
Copy link
Collaborator

Yeah. It was more convincing when I thought it could just take this as an argument, but tag and props seems good.

@jhicken
Copy link
Contributor Author

jhicken commented Jun 10, 2015

Is this what yall where thinking?

@jimfb
Copy link
Contributor

jimfb commented Jun 10, 2015

Thanks @jhicken! Looks great to me.

jimfb added a commit that referenced this pull request Jun 10, 2015
Ignore whitelisted attributes for native custom elements.
@jimfb jimfb merged commit 537a841 into facebook:master Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants