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

[core] Warn about Children.map & Fragment #12021

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 30, 2018

@oliviertassinari oliviertassinari merged commit c377e61 into mui:master Jun 30, 2018
@oliviertassinari oliviertassinari deleted the fragment- branch June 30, 2018 19:20
acroyear pushed a commit to acroyear/material-ui that referenced this pull request Jul 14, 2018
@nbkhope
Copy link

nbkhope commented Sep 6, 2018

I have a MenuList defined similar to the following:

<MenuList>
  <MenuItem>...</MenuItem>
  <MenuItem>...</MenuItem>
  {someConditionIsTrue && (
    <Fragment>
      <Divider />
      <MenuItem>...</MenuItem>
    </Fragment>
  }
</MenuList>

I use a React.Fragment at the end to conditionally add something extra to the list. There is no problem there. After upgrading MUI to a newer version (1.2.0 to 1.5.1.), I started getting the following warning:

warning.js:34 Warning: Material-UI: the MenuList component doesn't accept a Fragment as a child.
Consider providing an array instead.

Because of the changes in this PR. Everything is working fine, but I would like to get rid of this new warning. Is that possible? I feel like adding this warning is regressive in this case since Fragment is there so you don't add an extra <div>

@eps1lon
Copy link
Member

eps1lon commented Sep 7, 2018

@nbkhope Well have you considered using an array instead as the warning suggested? You could just define an array before and conditionally push new items.

@nbkhope
Copy link

nbkhope commented Sep 7, 2018

@eps1lon defining as an array require me add a "key" to every single entry. I feel like it gets more convoluted. I will just replace the Fragment with a div, but I still think doing this is regressive.

@eps1lon
Copy link
Member

eps1lon commented Sep 7, 2018

@nbkhope
The warning is not just to force some code style onto you. This is to pass some handlers to the children. You basically block this by wrapping them in a Fragment. This is explained in the linked issue in the description of this PR. Wrapping more than one relevant child might cause bugs depending on the behavior you expect.

@oliviertassinari
Copy link
Member Author

@nbkhope I want to remove the MenuList children loop in #10847. The current implementation has too many drawbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants