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

Fix react warning for instanceId prop on a DOM element (<Button> component) #14599

Merged
merged 7 commits into from
Mar 25, 2019

Conversation

nerrad
Copy link
Contributor

@nerrad nerrad commented Mar 24, 2019

Description

Was getting this warning appear whenever I clicked the menu toggle on a block:

Warning: React does not recognize the instanceId prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase instanceid instead. If you accidentally passed it from a parent component, remove it from the DOM element.

This results from the instanceId prop getting passed through via <MenuItem />

How has this been tested?

  • Verify unit tests/ e2e tests pass
  • Ensure console error doesn't appear in this branch.

Types of changes

Non-breaking change

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@nerrad nerrad self-assigned this Mar 24, 2019
@nerrad nerrad added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Mar 24, 2019
@nerrad nerrad added this to the 5.4 (Gutenberg) milestone Mar 24, 2019
@nerrad
Copy link
Contributor Author

nerrad commented Mar 24, 2019

I'm on the fence about this. An alternative way to approach this would be to review all components wrapped by the withInstanceId HOC and ensure they do not pass through the instanceId prop to children. What's unclear to me is whether there is current use-case in GB for passing through the instanceId prop, in which case that alternative might not be an option there.

Another alternative might be to create a HOC wrapping all leaf DOM elements to remove any props that shouldn't be a dom element attribute - so something like withCleanProps.

@gziolo
Copy link
Member

gziolo commented Mar 25, 2019

Another alternative might be to create a HOC wrapping all leaf DOM elements to remove any props that shouldn't be a dom element attribute - so something like withCleanProps.

This might be much nicer. However, it might have also some performance implications if you wrap all leaf components with such HOC. I'm wondering whether passing thru props a few levels down isn't the root cause of this in the first place.

@nerrad
Copy link
Contributor Author

nerrad commented Mar 25, 2019

The source of the instanceId appears to be the MenuItem component which is wrapped by withInstanceId. However, I don't see any actual use of instanceId in the MenuItem` component (or in the child components implemented there). So I'll give a go at simply removing that wrap there.

@nerrad
Copy link
Contributor Author

nerrad commented Mar 25, 2019

So turns out the <MenuItem> component did not need withInstanceId wrapping it. Removing that fixes the issue. This also required updating a couple snapshots in different packages.

@@ -1,3 +1,9 @@
7.2.1 (Unreleased)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is missing some ## :)

@nerrad nerrad merged commit 4c0d219 into master Mar 25, 2019
@nerrad nerrad deleted the BUG/fix-instanceId-prop-attribute-warning branch March 25, 2019 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants