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

ListItemSecondaryAction in MenuItem makes <li> inside <li> #10452

Closed
1 task done
fmy opened this issue Feb 26, 2018 · 3 comments
Closed
1 task done

ListItemSecondaryAction in MenuItem makes <li> inside <li> #10452

fmy opened this issue Feb 26, 2018 · 3 comments
Assignees
Labels
component: list This is the name of the generic UI component, not the React module! new feature New feature or request
Milestone

Comments

@fmy
Copy link

fmy commented Feb 26, 2018

ListItemSecondaryAction in MenuItem cause Warning: validateDOMNesting(...): <li> cannot appear as a descendant of <li>.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

inside component should be div.

Current Behavior

inside component becomes li.

Steps to Reproduce (for bugs)

https://codesandbox.io/s/2z43ov89vn

Context

ListItem has component and ContainerComponent props, but MenuItem has only component props.
To fix this warning, remove component defaultProps and add ContainerComponent defaultProps as li in MenuItem and pass it to ListItem (keep same props between these component).
Or, pass MenuItem's component props to ListItem as ContainerComponent.
related pr #10050

If you want, I will send pr to fix this.

Your Environment

Tech Version
Material-UI next(1.0.0-beta.35)
React latest(16.2.0)
browser any
etc
@mbrookes mbrookes added regression component: list This is the name of the generic UI component, not the React module! labels Feb 26, 2018
@oliviertassinari oliviertassinari added support: question Community support but can be turned into an improvement new feature New feature or request and removed regression support: question Community support but can be turned into an improvement labels Feb 26, 2018
@oliviertassinari
Copy link
Member

@fmy The issue comes from the fact that the MenuItem is asking his children to render as a li.
The ListItem is answering this requirement. But at the same time, the ListItemSecondaryAction usage requires the use of a wrapping li element. Hence li > li.

Workaround:

      <List>
-       <MenuItem>
+       <MenuItem component="div">
          <ListItemText primary="Inbox" />
          <ListItemSecondaryAction>
            <IconButton>
              <InboxIcon />
            </IconButton>
          </ListItemSecondaryAction>
        </MenuItem>
      </List>

We could look into improving the logic to prevent it in the first place.
Maybe:

Component = ContainerComponent === 'li' && Component === 'li' ? 'div' : Component;

Instead of:
https://github.com/mui-org/material-ui/blob/8898a8da3e29fbbeea514fd81123392da2afb8ea/src/List/ListItem.js#L116

@oliviertassinari oliviertassinari added this to the post v1.0.0 milestone Feb 26, 2018
@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Feb 26, 2018
@fmy
Copy link
Author

fmy commented Feb 27, 2018

@oliviertassinari Thanks, I will use this workaround.
This is just my opinion but I think MenuItem's component props should have same meaning with ListItem's one because they are inheritance relation.

@oliviertassinari
Copy link
Member

What make you think they don't have the same meaning? I think they do. The tricky part is the wrapping element that gets added for not nesting the buttons. Remove the MenuItem and you will get the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants