[Menu] Remove MenuItem dependency on ListItem

[jedwards1211]

A 4px margin was added to the top and bottom of ListItemText in #15339, causing an inconsistency between items that use it and items that just have a string child.

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The MenuItems in this JSX render with different heights when the viewport width >= 600:

    <MenuList>
      <MenuItem style={{ backgroundColor: "green" }}>
        without ListItemText
      </MenuItem>
      <MenuItem style={{ backgroundColor: "red" }}>
        <ListItemText>with ListItemText</ListItemText>
      </MenuItem>
    </MenuList>

When the viewport is narrower than 600px, a media query deactivates and their minHeight becomes 48px (ironically causing them to get taller on narrower screens...)

Expected Behavior 🤔

The menu items are the same height, like they used to be

Steps to Reproduce 🕹

https://codesandbox.io/s/blazing-resonance-oqiso?file=/src/Demo.js

Context 🔦

Discovered this working on material-ui-popup-state demos.

Your Environment 🌎

Refer to code sandbox for versions

[eps1lon]

Might be safe to switch to a padding instead and see what other components are affected by it.

[oliviertassinari]

@jedwards1211 I don't understand, why is this an issue? Why should it behave differently?

[jedwards1211]

I'm saying the two list items should not behave differently. In the past they didn't, but #15339 caused them to behave differently.

I don't know if the spec says anything about menu item height, but according to this quote from #6098 it doesn't sound like there should be any difference in height when using ListItemText vs. string children:

According to Material guidelines ALL(with controls also) single line list items must be 48px height and 40px when in dense mode. Only exception is when avatar element is present it can be 56px height.

Would reworking the minHeights and media queries on ListItem instead of using any padding or margin on the ListItemText work?

[oliviertassinari]

@jedwards1211 Ok, thanks for the extra information. I think that we will need to revamp the List and Menu implementation in v5. One element that has came up is that it's not a good idea to share the same style for both. So I think that we need to stop using this MenuItem > ListItem inheritance.

[jedwards1211]

Okay. This issue might exist with ListItem too, I need to check.

[siriwatknp]

@oliviertassinari for the upcoming ListItem improvement (no matter approach we take), I don't see a need to have MenuItem anymore except the a11y that it does.

// MenuItem.js
const {
    component = 'li',
    role = 'menuitem',
    tabIndex: tabIndexProp,
    ...other
  } = props;
  
  return (
    <ListItem
      role={role}
      tabIndex={tabIndex}
      component={component}
    />
  )

I see 2 ways.

1. Deprecate MenuItem in v5, change all the docs to use new ListItemButton (based on the outcome of improvement), remove MenuItem in v6. This ways, the a11y is on the developer responsibility and we have less component to maintain.
2. Remains MenuItem with CSS adjustment (by removing minHeight or media query) and let developer do the styling (nowadays it is easy via sx).

[oliviertassinari]

@siriwatknp I think that we will always need different components for rendering a list item, a menu item, and maybe a listbox item (select). Because it works in Material Design with the same component doesn't mean it will work in most design systems. I would definitely encourage 2 without any dependency on the list.