Material-ui: [List] Improve ListItemSecondaryAction handling

[tregusti]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

ListItem should consistently either render a <li> or not render a <li>

Current Behavior

When adding a ListItemSecondaryAction into the ListItem a <li> is automatically added. If I render a list where some items should have an icon and some do not, this element structure becomes inconsistent.

Steps to Reproduce

Link: https://codesandbox.io/s/20w105myyy

1. Open Browser DevTools and inspect the DOM structure or the children to the <ul>s.

Context

I'm trying to make a generic ListItemLink helper that wraps the content inside a ListItem and a NavLink like this:

class ListItemLink extends Component {
  render() {
    const { classes, to, children } = this.props;

    return (
      <ListItem button component={NavLink} to={to} activeClassName={classes.active}>
        {children}
      </ListItem>
    );
  }
}

But since I use component property I override the default use of <li>, however, when adding the ListItemSecondaryAction inside of it a <li> is added. I assume that this is somehow related to #10452, but cannot understand how to fix it. Is this a bug? If not, how can I work aroun this issue when I do not know beforehand if the content will contain a ListItemSecondaryAction?

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | v3.4.0 |
| React | v16.6 |
| React Router | 4.3.1 |

[oliviertassinari]

@tregusti Your HTML5 semantic isn't valid in your example. It should be ul > li > a.

        </ListItem>
+       <li>
          <ListItem button component={NavLink} to="/dfgafdg">
            <ListItemText primary="I do not have a li" />
          </ListItem>
+       </li>
      </List>

We are using the following logic on Material-UI side to determine the existance of a secondary action:

const children = React.Children.toArray(childrenProp);
const hasSecondaryAction =
  children.length &&
  isMuiElement(children[children.length - 1], ['ListItemSecondaryAction']);

You can do the same.

[tregusti]

That is exactly my problem, hence the reason I opened the issue...

If I loop a bunch of items that will contain the information that renders the links. Sometimes I should _manually_ add the <li> and sometimes not, and that is my problem.

As I see it, ListItem should either always add a <li> or never do it. "Sometimes" it not really something that is easy to handle as a consumer of an API.

It the following code, the generated markup will become non-semantic, since <li> will sometimes be present and sometimes not.

<nav>
    <List>
        { items.map(item => (
            <ListItem ...>
                {/*
                    content based of `item`.
                    sometimes with and sometimes without secondary action
                */}
            </ListItem>
        ))}
    </List>
</nav>

If what you suggest is how to do it, then I should take into account how internal code in material-ui looks/works instead of assuming it is consistent. So if this is not a bug, then I would either like a property available telling me if a <li> will be automatically added or not so that I can add it myself. I should NOT check for the presence of ListItemSecondaryAction because that is how the internal code in the lib currently works.

So please reconsider closing this. Or at least let some mor eyes see it before closing it.

[eps1lon]

TL;DR: I agree the code is bloated but that markup is weird in the first place. It equates to buttons inside buttons.

I had the same issue when porting the context. It his very hard to reason about a component if that component changes the markup depending on the immediate children. It is very close to breaking component encapsulation and doesn't even handle cases where one would wrap ListItemSecondaryAction in a headless component.

That being said I find the component itself an example where we trade reasonable code for writing shorthand code that does not make sense semantically in the end.

What should've happened is a separate ListItemButton that doesn't allow anything else than text as a child. It doesn't make much sense to have a <ListItem button /> that also has a secondary action. Buttons inside Buttons seems like a very bad idea. That is also closer to the W3 spec that states that elements should not change their roles over time (which we basically enable with a button prop).

PS: I'm totally aware that I might miss many use cases here and look at this from a very simplified perspective but I believe that I rather have a many one or two prop components than a 10 prop component that allows for nonsensical combinations of those.

[oliviertassinari]

That is exactly my problem, hence the reason I opened the issue...

I'm raising the issue because your demo states that ul > li > a is wrong and that ul > a is correct. It's the oppossite.

Sometimes I should manually add the

and sometimes not, and that is my problem.

There is an intrinsic complexity with nesting buttons and links. Of course. We could make the API more explicit as @eps1lon is suggesting but it won't solve your problem, you still have to code branch the two cases. The upside of @eps1lon's approach is that it's explicit. No surprises, this is great. I have spend hours into try to get the current button nesting logic "right". I understand your frustration, it's a pain to get it right!

So, going forward:

1. You can add a property to your custom list item to know when to add a li and when not to.
2. We can look into increasing the API surface of the list item, to make it closer to the underlying DOM node. I has a great potential. However, at this point, I don't think that it will better solve your problem, you have already been hit by the need of code branch.

[tregusti]

First off, I really do appreciate all the time you put into the library!

So, maybe my demo isn't as clear as I wanted it. I'm not advocating ul > a. At least that was not my intent.

So, my real problem is when using <ListItem component={NavLink}> I assume that I tell ListItem to NOT render a <li> since I explicitly want a NavLink instead. And then of course I have to manually add the <li> myself, that it fine. What is not fine is that I get another nested <li> when adding the ListItemSecondaryAction into my content of the ListItem.

I updated the demo with a new list in the end, clarifying the problem (I hope).

https://codesandbox.io/s/20w105myyy

For now however, I will go forward as you suggested in your comment.

Note to self, do not copy/paste a demo for something else, start from scratch =)

[ryancogswell]

@tregusti When a ListItemSecondaryAction is in the ListItem, the component gets wrapped by the ContainerComponent. This defaults to li, but it is another property that you can control. So, for instance, you could specify ContainerComponent to be div instead of li (e.g. <ListItem component={NavLink} ContainerComponent="div">) and then know that you can safely wrap all your list items in li without resulting in nested lis.

You can specify this ContainerComponent unconditionally (without needing to know whether or not your ListItem contains a ListItemSecondaryAction) because it will be ignored except in the case where a ListItemSecondaryAction is detected.

[tregusti]

Thank you so much @ryancogswell. This was what I was after (next to a more consistent behaviour =)

I will try this out!

[ryancogswell]

@tregusti Another option to get more consistent markup across your list items would be to always include ListItemSecondaryAction and just have it be empty when it isn't applicable. This will have the effect of leaving a space at the end where an icon would be (due to a paddingRight: 32 that is added when the ListItem has a ListItemSecondaryAction) which, depending on what is in the list items, may be a desirable look.

Here's an example of this (via modifying a CodeSandbox from another ListItem issue):

[ilovett]

For me I was just trying to use the list item styling without being in a List

The following will correctly render div as I want with component="div"

<ListItem component="div" disableGutters>
  <ListItemAvatar>
    <Avatar>{ avatarContent }</Avatar>
  </ListItemAvatar>
  <ListItemText
    primary="ListItem is a div"
  />
</ListItem>

As soon as I have a ListItemSecondaryAction child, it renders as li and ignored component="div"

<ListItem component="div" disableGutters>
  <ListItemAvatar>
    <Avatar>1</Avatar>
  </ListItemAvatar>
  <ListItemText
    primary="ListItem is a li"
  />
  <ListItemSecondaryAction>
    <IconButton edge="end">
      <OpenInNewIcon />
    </IconButton>
  </ListItemSecondaryAction>
</ListItem>

[eps1lon]

I think we need to revisit this for v5. The current situation with regards to navs is pretty bad (our "simple" demo is not how you'd mark up a navigation UI). It's too easy to get wrong e.g.

<nav>
  <List>
    <ListItem button component={Link} href="/">
      Foo
    </ListItem>
  </List>
</nav>

creates invalid and wrong markup.

<nav>
  <List>
+   <li>
      <ListItem button component={Link} href="/">
        Foo
      </ListItem>
+   </li>
  </List>
</nav>

is unintuitive (li is a shorthand for listitem; do I have a button or a link?).

It's probably best always add an li and then nest a dedicated ListItemLink e.g.

<nav>
  <List>
    <ListItem>
      <ListItemButton component={NavLink}>
        Foo
      </ListItemButton>
      <ListItemSecondaryAction>
        Bar
      </ListItemSecondaryAction>
    </ListItem>
  </List>
</nav>

This still has the button vs link problem but at least that's (for better or worse) well established in Material-UI. The upside is that this naturally creates the correct UI (nav > ul > li > a) and nested buttons are discouraged.

[oliviertassinari]

Agree, I think that abstracting two DOM nodes in the initial design was too ambitious, in this context.