Skip to content

Conversation

@eins78
Copy link
Contributor

@eins78 eins78 commented Jul 1, 2014

very small change, but once you start listing things that can't be nested <form> should be among them ;)

@zpao
Copy link
Member

zpao commented Jul 3, 2014

I like this, but please run the test before committing, this is failing (we check the error message).

https://github.com/facebook/react/blob/master/src/core/__tests__/ReactInstanceHandles-test.js#L141-L154

@sophiebits
Copy link
Collaborator

Note that this same error message is hardcoded in another place – can you update both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit: do you mind adding a comma after <p>?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

one more tiny nit: can you move the space between "parent." and "Try" to the previous line for consistency with the other line breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spicyj If I move the space, It would go exatly 1 character over the line limit (80chars),
so it would add another line…

Edit: doesn't really matter because the extra comma already means 1 more line is needed.

@eins78
Copy link
Contributor Author

eins78 commented Jul 4, 2014

Thanks for the feedback, it would not have occured to me to run test for a wording change, but of course it makes sense :)
I just fixed the test, but while grepping for "unexpectedly mutated" to find it I also came across another file which has the same error message (but in another context):
src/browser/ui/dom/DOMChildrenOperations.js
Is this duplication wanted? If yes, I should probably update the message there too, right?

@zpao
Copy link
Member

zpao commented Jul 7, 2014

Oh yea, I missed that - good find. Let's update both places.

@eins78
Copy link
Contributor Author

eins78 commented Jul 8, 2014

@zpao Cool, I updated it.
Mind that this message seems to be not tested (not sure if this is an issue).

zpao added a commit that referenced this pull request Jul 24, 2014
Also list <form> in error message as possible culprit
@zpao zpao merged commit f76d4dd into facebook:master Jul 24, 2014
@zpao
Copy link
Member

zpao commented Jul 24, 2014

Sorry for the delay! Thanks!

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.

3 participants