Improve documentation for ListItem and ListItemSecondaryAction

[fastfedora]

Improve the documentation for how ListItemSecondaryAction components work within ListItem components.

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

Expected Behavior 🤔

The documentation would have warnings / implementation details indicating the following:

• ListItemSecondaryAction must be the last child of ListItem for ListItem to detect it has a secondary action (see source code).
• When using a wrapper around ListItemSecondaryAction, the muiName field must be set to 'ListItemSecondaryAction' (see code above and issue Cannot use wrapper for ListItemSecondaryAction #11622).
• When using a ListItemSecondaryAction within a ListItem, the component used for the ListItem changes (see issue [List] Improve ListItemSecondaryAction handling #13597).

These would be useful at the top of the API documentation for ListItemSecondaryAction and possibly ListItem.

Current Behavior 😯

These behaviors are undocumented and require hunting through the issues and the code to figure out.

Context 🔦

I spent all morning trying to figure out why when I hovered or clicked on my secondary action, it also triggered the list item. I finally figured out it was the order of the children after looking at the code. I was also using a wrapper for ListItemSecondaryAction, which had to be fixed too.

My use case is that I wanted the secondary action on the left side, not the right side. So I had changed the order of the children, not thinking that just changing the order would affect the behavior of ListItem.

Long-term, the best solution would be to improve the implementation so secondary actions are not so fragile. But I understand there may be other priorities right now.

One possibility to explore for a future API is to have the secondary action passed in via a secondaryActionComponent prop. Then the ListItem would always know whether a secondary action existed and could wrap it within a ListItemSecondaryAction internally.

[eps1lon]

1. is addressed by [ListItem] Improve ListItemSecondaryAction DX #14350
2. is explained in Cannot use wrapper for ListItemSecondaryAction #11622 (comment). We already have a section about this. I don't think it's very helpful to link this section from every affected component. I would open a separate PR that improves the documentation so that users are able to identify the issue more easily.
3. This is somewhat hidden in https://material-ui.com/api/list-item/#props but I improved the docs in [ListItem] Improve ListItemSecondaryAction DX #14350 concerning this issue too.

[jedwards1211]

This is one of the worst pain points I've encountered in MUI...

I was encountering crazy back-and-forth scroll behavior with a react-virtualized list in which I was rendering ListItems. After an hour of debugging, I discovered it was because the style I was passing from react-virtualized to the ListItem wasn't winding up on its root element, causing all hell to break loose because that style dictates relative position for each item.

Essentially, to use a component that renders a ListItem successfully with react-virtualized, you have to know whether that component renders a secondary action, and

• Pass style={reactVirtualizedStyle} if it has no secondary action
• Pass ContainerProps={{style: reactVirtualizedStyle}} if it does have a secondary action

I fully understand that supporting the base case, the button case, and the custom component case may require some ugly tradeoffs. But <AnyComponent style={style}> passing the style to an inner element under any circumstances will always be a gotcha, and maybe other potential tradeoffs don't have that kind of gotcha. I don't suppose it would be possible to do the secondary action layout within the root child passed to the component of <ListItem component={...}>?

I ended up making this workaround component...not completely sure how I feel about it. I might even make it a global component override.

/**
 * @flow
 * @prettier
 */

import * as React from 'react'
import ListItem from '@material-ui/core/ListItem'
import { isMuiElement } from '@material-ui/core/utils'

export type Props = {
  +children?: any,
  +style?: ?Object,
  +ContainerProps?: ?Object,
}

/* eslint-disable react/display-name */
const VirtualizedListItem = React.forwardRef(
  (
    { children: childrenProp, style, ContainerProps, ...props }: Props,
    ref: React.ElementRef<any>
  ): React.Node => {
    const children = React.Children.toArray(childrenProp)
    const hasSecondaryAction =
      children.length &&
      isMuiElement(children[children.length - 1], ['ListItemSecondaryAction'])

    return (
      <ListItem
        {...props}
        ref={ref}
        ContainerProps={
          hasSecondaryAction
            ? {
                ...ContainerProps,
                style: { ...ContainerProps?.style, ...style },
              }
            : ContainerProps
        }
        style={hasSecondaryAction ? { height: style?.height } : style}
      >
        {children}
      </ListItem>
    )
  }
)

export default VirtualizedListItem

[oliviertassinari]

I believe we have another open issue dedicated to this topic (labeled breaking change). The best solution is likely to introduce a new component, to keep the user API close to the underlying DOM structure.

[oliviertassinari]

See #13597

[jedwards1211]

Okay great! I didn't manage to find that. Yeah a new component was one potential solution I had in mind.