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

Minor updates to findDOMNode, as per additional po #2800

Merged
merged 1 commit into from
Jan 5, 2015

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Jan 2, 2015

Minor updates to findDOMNode, as per additional post-commit CR feedback.

@@ -36,8 +34,8 @@ function findDOMNode(componentOrElement) {
return ReactMount.getNodeFromInstance(componentOrElement);
}
invariant(
!(componentOrElement.render != null && typeof(componentOrElement.render) === 'function'),
'Component contains `render` method but is not mounted in the DOM',
componentOrElement.render == null || typeof(componentOrElement.render) !== 'function',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still like the un-demorganized form better. What do people think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if most of the times in the source we see invariant(condition), it's better to have it that way without negations. I think demorganized version reads more easily.

Copy link
Member

Choose a reason for hiding this comment

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

I obviously like the demorganized. Also, this is > 80 chars, can you split at the || to quiet one more lint complaint.

@jimfb jimfb force-pushed the getDOMNode-becomes-findDOMNode branch from 1d59155 to f507ba3 Compare January 5, 2015 21:26
jimfb added a commit that referenced this pull request Jan 5, 2015
Minor updates to findDOMNode, as per additional post-commit CR feedback.
@jimfb jimfb merged commit 2a28189 into facebook:master Jan 5, 2015
@jimfb jimfb deleted the getDOMNode-becomes-findDOMNode branch January 5, 2015 21:29
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