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

group warnings for unknown DOM properties #7153

Merged
merged 1 commit into from
Jun 30, 2016

Conversation

griffinmichl
Copy link
Contributor

@griffinmichl griffinmichl commented Jun 30, 2016

#7152. May need to refactor, but I think this is the idea.

tagName,
ReactComponentTreeDevtool.getStackAddendumByID(debugID)
);
unknownProp = name;
Copy link
Collaborator

@gaearon gaearon Jun 30, 2016

Choose a reason for hiding this comment

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

The calling function already knows the name of the unknown prop—it passes it as an argument. Let’s just return true if there is no need for additional warning, and false if we still need to warn. We can rename handleProperty to validateProperty.

@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2016

Thanks! I left a few comments about refactoring but this mostly looks good!

@griffinmichl
Copy link
Contributor Author

@gaearon Thanks for the feedback. I have incorporated your suggestions.

@@ -75,6 +75,8 @@ if (__DEV__) {
standardName,
ReactComponentTreeDevtool.getStackAddendumByID(debugID)
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: let’s remove this newline.

@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2016

Accepting with nits. Please fix those and I’ll merge. Thanks!

@gaearon gaearon added this to the 15.2.0 milestone Jun 30, 2016
@griffinmichl
Copy link
Contributor Author

Nits fixed.

@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2016

Could you try this test:

  1. clone react-bootstrap
  2. run its docs website locally
  3. run npm run build in your React folder
  4. replace react-bootstrap/node_modules/react/lib/* with react/build/modules/* from your build
  5. ensure the warnings are grouped correctly?

A screenshot verifying it would be most welcome 😄

@griffinmichl
Copy link
Contributor Author

Sure thing!

screen shot 2016-06-30 at 3 50 15 pm

@gaearon gaearon merged commit 39265cb into facebook:master Jun 30, 2016
zpao pushed a commit that referenced this pull request Jul 1, 2016
@griffinmichl griffinmichl deleted the unknownWarnings branch July 1, 2016 19:01
usmanajmal pushed a commit to usmanajmal/react that referenced this pull request Jul 11, 2016
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.

None yet

2 participants