[RFC] ListItem improvement

[siriwatknp]

Problem

Regarding to #13597

The implementation of ListItem is complex to understand and does too many things that developers does not expect.

• render ListItemSecondaryAction inside ListItem cause the markup to change

  <ListItem>
    Some text
  </ListItem>
  // result
  // <div>Some text</div>
  
  <ListItem>
    Some text
    <ListItemSecondaryAction>
      action
    </ListItemSecondaryAction>
  </ListItem>
  // result
  // <div>
  //   <div>Some text</div>
  //   <button>action</div>
  // </div>

  This cause a lot of confusion to developer. Moreover, the ContainerProps is added for customization which add more API for the developer to know.

• when button prop is true, ButtonBase is used which change the default tag of ListItem, again cause confusion and developer needs to know the implementation detail

  <ListItem>
    Some text
  </ListItem>
  // result => <li>Some text</li>
  
  <ListItem button>
    Some text
  </ListItem>
  // result => <div>Some text<div>

• Frequently, the ListItem is override by a or Link from router library. Developer needs to know to wrap with li

  <li>
    <ListItem button component={Link}>
  	Some text
    </ListItem>
  </li>

Goal

List components tends to be used to create new component like nested menu, link item, and many more. So, material-ui should provide the list components that are styled with material design spec and simple to compose together without needing to know much about the implementation.

• no children checking
• no DOM changing between prop
• no container props

Solution I : One element, One DOM #26446

From comments and issues, one way to improved is to extract button out of ListItem so each component render only single markup

// v4
<ListItem button>...</ListItem>

// v5
<ListItem>
  <ListItemButton>...</ListItemButton>
</ListItem>

Pros

• least API (no nested component API)
• types is straight forward
• replacing with routing library is similar to Button

Cons

• Developer needs to know the markup before picking the component
• using ListItem and ListItemButton together require nested CSS & negative margin. (customise padding will require negative margin adjustment)

Solution II : Flat ListItemButton #26480

ListItemButton always render 2 DOM, by default <li> <div>

// v4
<ListItem button>...</ListItem>

// v5
<ListItemButton>...</ListItemButton>

Pros

• no nested css specificity, custom styles is straight forward

Cons

• hard to manage types
• overriding via componentsProps

Secondary Action

Both solutions will use the same API for handling secondary action. Regardless of the approach, ListItem and ListItemButton accept secondaryAction prop.

<ListItem secondaryAction={...}>

<ListItemButton secondaryAction={...}>

This will remove the hidden logic to check if children has ListItemSecondaryAction or not.

Breaking change?

Instead of making this as breaking change , I prefer to adjust ListItem and mark some prop like button as deprecated and then remove them in next major version because I assume that most of the project use ListItem in some way.

Further

from @eps1lon suggestion, it is nice to expose UnorderedList and OrderedList as a wrapper of ListItem like Chakra-UI does.

cc @mui-org/core

[oliviertassinari]

Sounds good.

[mnajdova]

I would vote for Solution I, it doesn't have any magic behind it, and with good docs demos, developers shouldn't have any problems implementing it.

[oliviertassinari]

I would vote for Solution I. To keep customizations as simple as possible. Regarding the cons.

Developer needs to know the markup before picking the component

I think that the audience that doesn't know the correct markup is also the audience that copy and paste the demos a lot, so it doesn't matter. The audience that knows the correct markup will be destabilized by seeing a component host two DOM nodes.

using ListItem and ListItemButton together require nested CSS & negative margin. (customise padding will require negative margin adjustment)

Agree, best to avoid negative margin, it can be destabilizing.

[michaldudak]

I could live with either but think that solution 2 reduces boilerplate code and makes the simple usage scenario (no customisation) quicker to implement, so my vote goes there.

[oliviertassinari]

I think that the framing in this RFC's description misses an important dimension. How do we make the item interactive? It either need a click listener as a button or a <a> and systematically one of the two.

With Solution I:

<List>
  <ListItem>
    <ListItemButton component={Link} to="/foo">
    </ListItemButton>
  </ListItem>
</List>

With Solution II, we need to take #21453, we need:

<List>
  <ListItemButton componentsProps={{ button: { component: 'a', href: '#id' } }}>
  </ListItemButton>
</List>

If we render a link, or if we render a button

<List>
  <ListItemButton componentsProps={{ button: { onClick: handleClick } }}>
  </ListItemButton>
</List>

[siriwatknp]

Agree, best to avoid negative margin, it can be destabilizing.

What do you propose without using negative margin in this case (and without js to check)?

<ListItem>
  <ListItemButton>
    Some text
  </ListItemButton>
</ListItem>

Can it be something explicit by user?

<ListItem disablePadding>
  <ListItemButton>
    Some text
  </ListItemButton>
</ListItem>

[oliviertassinari]

What do you propose without using negative margin in this case (and without js to check)?

@siriwatknp No preferences, we could try with a negative magin. I was simply confirming your point, that it's not awesome.

[siriwatknp]

To push this forward, I suggest we go with solution 1 for v5 and keep an eyes for related API issues

• I think people likely to pass props to ListItemButton so solution 1 looks cleaner

  <ListItem>
    <ListItemButton sx={{...}} component={Link} href="/" onClick={() => {}} />
  </ListItem>
  
  // whereas
  <ListItemButton componentsProps={{ button: { component: Link, href: "/", sx: {...}, onClick: () => {} } }} />

• Developer can compose by themselves if their feel too much boilerplate for solution 1.

  const ListActionItem = () => (
    <ListItem>
      <ListItemButton />
    </ListItem>
  )
  
  <ListActionItem />

  Once v5 is stable and there are complaints about this, we can expose another component like ListActionItem above.

I suggest to not spend too much time (it's been over a week already) on this because both ways are far better than the v4 approach. Let's try and see the feedback from community.

cc @oliviertassinari @mnajdova @michaldudak @eps1lon

[eps1lon]

I suggest to not spend too much time (it's been over a week already) on this because both ways are far better than the v4 approach. Let's try and see the feedback from community.

Sounds good 👍