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

Fixed removing attributes during custom element update. Fixes #6747 #6748

Merged

Conversation

grassator
Copy link
Contributor

@grassator grassator commented May 11, 2016

I've tried to follow the same approach as the nextProps takes, which involved adding a new method to explicitly delete the attribute without all the checks React does.

I would be happy to any necessary changes and hope it can be merged soon.

Thanks

Fixes #6747.

@jimfb
Copy link
Contributor

jimfb commented May 11, 2016

Looks good to me ( 👍 )

@grassator
Copy link
Contributor Author

@jimfb Thanks for a such a quick review!

I would really appreciate if you could let me know on the next steps so I can either setup our infrastructure around my fork, or wait for merge to master / next release. Thanks in advance.

@jimfb jimfb merged commit 0e889d7 into facebook:master May 13, 2016
@jimfb
Copy link
Contributor

jimfb commented May 13, 2016

@grassator Merged into master. This should make it into one of the upcoming releases.

@grassator
Copy link
Contributor Author

@jimfb Thanks a lot for an expedited handling — it's really appreciated.

node.removeAttribute(name);
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onDeleteValueForProperty(node, name);
ReactInstrumentation.debugTool.onNativeOperation(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: this method was renamed in #6754 which causes tests to fail now.

gaearon added a commit that referenced this pull request May 14, 2016
We had a rename in #6754 but #6748 got merged after that referencing the old name.
@grassator
Copy link
Contributor Author

@gaearon sorry about that—I didn't know about upcoming rename.

@gaearon
Copy link
Collaborator

gaearon commented May 14, 2016

Yea, no problem!

@zpao zpao added this to the 15-next milestone Jun 1, 2016
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
zpao pushed a commit that referenced this pull request Jun 14, 2016
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
jscissr added a commit to jscissr/react-polymer that referenced this pull request Jul 5, 2016
Fix after facebook/react#6748 broke the monkey-patching
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.

4 participants