[ListItem] Improve ListItemSecondaryAction DX (#14350)

* [ListItem] Warn against wrong position for secondary action

* [docs] Improve documentation of ListItemSecondaryAction interactions

* Update ListItem.js

* Update ListItem.test.js

* Update list-item.md

Author: eps1lon

packages/material-ui/src/ListItem/ListItem.js

@@ -1,7 +1,7 @@
 import React from 'react';
 import PropTypes from 'prop-types';
 import classNames from 'classnames';
-import { componentPropType } from '@material-ui/utils';
+import { chainPropTypes, componentPropType } from '@material-ui/utils';
 import withStyles from '../styles/withStyles';
 import ButtonBase from '../ButtonBase';
 import { isMuiElement } from '../utils/reactHelpers';
@@ -83,6 +83,9 @@ export const styles = theme => ({
   selected: {},
 });
 
+/**
+ * Uses an additional container component if `ListItemSecondaryAction` is the last child.
+ */
 function ListItem(props) {
   const {
     alignItems,
@@ -179,9 +182,35 @@ ListItem.propTypes = {
    */
   button: PropTypes.bool,
   /**
-   * The content of the component.
+   * The content of the component. If a `ListItemSecondaryAction` is used it must
+   * be the last child.
    */
-  children: PropTypes.node,
+  children: chainPropTypes(PropTypes.node, props => {
+    const children = React.Children.toArray(props.children);
+
+    // React.Children.toArray(props.children).findLastIndex(isListItemSecondaryAction)
+    let secondaryActionIndex = -1;
+    for (let i = children.length - 1; i >= 0; i -= 1) {
+      const child = children[i];
+      if (isMuiElement(child, ['ListItemSecondaryAction'])) {
+        secondaryActionIndex = i;
+        break;
+      }
+    }
+
+    //  is ListItemSecondaryAction the last child of ListItem
+    if (secondaryActionIndex !== -1 && secondaryActionIndex !== children.length - 1) {
+      return new Error(
+        'Material-UI: you used an element after ListItemSecondaryAction. ' +
+          'For ListItem to detect that it has a secondary action ' +
+          `you must pass it has the last children to ListItem.${
+            process.env.NODE_ENV === 'test' ? Date.now() : ''
+          }`,
+      );
+    }
+
+    return null;
+  }),
   /**
    * Override or extend the styles applied to the component.
    * See [CSS API](#css-api) below for more details.
@@ -198,12 +227,11 @@ ListItem.propTypes = {
    */
   component: componentPropType,
   /**
-   * The container component used when a `ListItemSecondaryAction` is rendered.
+   * The container component used when a `ListItemSecondaryAction` is the last child.
    */
   ContainerComponent: componentPropType,
   /**
-   * Properties applied to the container element when the component
-   * is used to display a `ListItemSecondaryAction`.
+   * Properties applied to the container component if used.
    */
   ContainerProps: PropTypes.object,
   /**

packages/material-ui/src/ListItem/ListItem.test.js

@@ -1,6 +1,7 @@
 import React from 'react';
 import { assert } from 'chai';
 import { getClasses, createMount, findOutermostIntrinsic } from '@material-ui/core/test-utils';
+import consoleErrorMock from 'test/utils/consoleErrorMock';
 import ListItemText from '../ListItemText';
 import ListItemSecondaryAction from '../ListItemSecondaryAction';
 import ListItem from './ListItem';
@@ -166,6 +167,31 @@ describe('<ListItem />', () => {
       assert.strictEqual(listItem.hasClass(classes.container), true);
       assert.strictEqual(listItem.hasClass('bubu'), true);
     });
+
+    describe('warnings', () => {
+      beforeEach(() => {
+        consoleErrorMock.spy();
+      });
+
+      afterEach(() => {
+        consoleErrorMock.reset();
+      });
+
+      it('warns if it cant detect the secondary action properly', () => {
+        mount(
+          <ListItem>
+            <ListItemSecondaryAction>I should have come last :(</ListItemSecondaryAction>
+            <ListItemText>My position doesn not matter.</ListItemText>
+          </ListItem>,
+        );
+
+        assert.strictEqual(consoleErrorMock.callCount(), 1);
+        assert.include(
+          consoleErrorMock.args()[0][0],
+          'Warning: Failed prop type: Material-UI: you used an element',
+        );
+      });
+    });
   });
 
   describe('prop: focusVisibleClassName', () => {

packages/material-ui/src/ListItemSecondaryAction/ListItemSecondaryAction.js

@@ -13,6 +13,9 @@ export const styles = {
   },
 };
 
+/**
+ * Must be used as the last child of ListItem to function properly.
+ */
 function ListItemSecondaryAction(props) {
   const { children, classes, className, ...other } = props;
 

pages/api/list-item-secondary-action.md

@@ -12,7 +12,7 @@ filename: /packages/material-ui/src/ListItemSecondaryAction/ListItemSecondaryAct
 import ListItemSecondaryAction from '@material-ui/core/ListItemSecondaryAction';
 ```
 
-
+Must be used as the last child of ListItem to function properly.
 
 ## Props
 

pages/api/list-item.md

@@ -12,19 +12,19 @@ filename: /packages/material-ui/src/ListItem/ListItem.js
 import ListItem from '@material-ui/core/ListItem';
 ```
 
-
+Uses an additional container component if `ListItemSecondaryAction` is the last child.
 
 ## Props
 
 | Name | Type | Default | Description |
 |:-----|:-----|:--------|:------------|
 | <span class="prop-name">alignItems</span> | <span class="prop-type">enum:&nbsp;'flex-start'&nbsp;&#124;<br>&nbsp;'center'<br></span> | <span class="prop-default">'center'</span> | Defines the `align-items` style property. |
 | <span class="prop-name">button</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the list item will be a button (using `ButtonBase`). |
-| <span class="prop-name">children</span> | <span class="prop-type">node</span> |   | The content of the component. |
+| <span class="prop-name">children</span> | <span class="prop-type">node</span> |   | The content of the component. If a `ListItemSecondaryAction` is used it must be the last child. |
 | <span class="prop-name">classes</span> | <span class="prop-type">object</span> |   | Override or extend the styles applied to the component. See [CSS API](#css-api) below for more details. |
 | <span class="prop-name">component</span> | <span class="prop-type">Component</span> |   | The component used for the root node. Either a string to use a DOM element or a component. By default, it's a `li` when `button` is `false` and a `div` when `button` is `true`. |
-| <span class="prop-name">ContainerComponent</span> | <span class="prop-type">Component</span> | <span class="prop-default">'li'</span> | The container component used when a `ListItemSecondaryAction` is rendered. |
-| <span class="prop-name">ContainerProps</span> | <span class="prop-type">object</span> |   | Properties applied to the container element when the component is used to display a `ListItemSecondaryAction`. |
+| <span class="prop-name">ContainerComponent</span> | <span class="prop-type">Component</span> | <span class="prop-default">'li'</span> | The container component used when a `ListItemSecondaryAction` is the last child. |
+| <span class="prop-name">ContainerProps</span> | <span class="prop-type">object</span> |   | Properties applied to the container component if used. |
 | <span class="prop-name">dense</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, compact vertical padding designed for keyboard and mouse input will be used. |
 | <span class="prop-name">disabled</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the list item will be disabled. |
 | <span class="prop-name">disableGutters</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the left and right padding is removed. |