[ListItemAvatar] Fix & refactor (#6540)

- It didn't work at all - wrong MUI_SHEET_ORDER - If it had worked, the avatar was the wrong size, and the icon wasn't resized - Simplified usage - styles conditionaly aapplied if dense context is true - Updated the error message - Fixed the tests - Simplified the examples.
mui · Apr 8, 2017 · 53a8fb1 · 53a8fb1
1 parent 17f0f84
commit 53a8fb1
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 31 deletions.
diff --git a/docs/src/pages/component-demos/lists/InteractiveList.js b/docs/src/pages/component-demos/lists/InteractiveList.js
@@ -117,17 +117,11 @@ class InteractiveList extends Component {
               <List dense={dense}>
                 {generate((
                   <ListItem button>
-                    {dense ? (
-                      <ListItemAvatar>
-                        <Avatar>
-                          <FolderIcon />
-                        </Avatar>
-                      </ListItemAvatar>
-                    ) : (
+                    <ListItemAvatar>
                       <Avatar>
                         <FolderIcon />
                       </Avatar>
-                    )}
+                    </ListItemAvatar>
                     <ListItemText
                       primary="Single-line item"
                       secondary={secondary ? 'Secondary text' : null}
@@ -145,17 +139,11 @@ class InteractiveList extends Component {
               <List dense={dense}>
                 {generate((
                   <ListItem button>
-                    {dense ? (
-                      <ListItemAvatar>
-                        <Avatar>
-                          <FolderIcon />
-                        </Avatar>
-                      </ListItemAvatar>
-                    ) : (
+                    <ListItemAvatar>
                       <Avatar>
                         <FolderIcon />
                       </Avatar>
-                    )}
+                    </ListItemAvatar>
                     <ListItemText
                       primary="Single-line item"
                       secondary={secondary ? 'Secondary text' : null}

diff --git a/src/Avatar/Avatar.js b/src/Avatar/Avatar.js
@@ -99,7 +99,7 @@ Avatar.propTypes = {
   /**
    * @ignore
    * The className of the child element.
-   * Used by Chip to style the Avatar icon.
+   * Used by Chip and ListItemIcon to style the Avatar icon.
    */
   childrenClassName: PropTypes.string,
   /**

diff --git a/src/List/ListItemAvatar.js b/src/List/ListItemAvatar.js
@@ -8,10 +8,15 @@ import customPropTypes from '../utils/customPropTypes';
 
 export const styleSheet = createStyleSheet('MuiListItemAvatar', () => {
   return {
-    dense: {
-      height: 32,
-      marginRight: 8,
-      width: 32,
+    denseAvatar: {
+      width: 36,
+      height: 36,
+      fontSize: 18,
+      marginRight: 4,
+    },
+    denseAvatarIcon: {
+      width: 20,
+      height: 20,
     },
   };
 });
@@ -26,9 +31,9 @@ export const styleSheet = createStyleSheet('MuiListItemAvatar', () => {
  * ```
  */
 export default function ListItemAvatar(props, context) {
-  if (!context.dense) {
+  if (context.dense === undefined) {
     warning(false, `Material-UI: <ListItemAvatar> is a simple wrapper to apply the dense styles 
-      to Avatar. You do not need it.`);
+      to <Avatar>. You do not need it unless you are controlling the <List> dense property.`);
     return props.children;
   }
 
@@ -40,7 +45,15 @@ export default function ListItemAvatar(props, context) {
   const classes = context.styleManager.render(styleSheet);
 
   return React.cloneElement(children, {
-    className: classNames(classes.dense, classNameProp, children.props.className),
+    className: classNames(
+      { [classes.denseAvatar]: context.dense },
+      classNameProp,
+      children.props.className,
+    ),
+    childrenClassName: classNames(
+      { [classes.denseAvatarIcon]: context.dense },
+      children.props.childrenClassName,
+    ),
     ...other,
   });
 }

diff --git a/src/List/ListItemAvatar.spec.js b/src/List/ListItemAvatar.spec.js
@@ -5,6 +5,7 @@ import React from 'react';
 import { assert } from 'chai';
 import { createShallow } from 'src/test-utils';
 import ListItemAvatar, { styleSheet } from './ListItemAvatar';
+import Avatar from '../Avatar';
 
 describe('<ListItemAvatar />', () => {
   let shallow;
@@ -15,23 +16,23 @@ describe('<ListItemAvatar />', () => {
     classes = shallow.context.styleManager.render(styleSheet);
   });
 
-  it('should render a span', () => {
+  it('should render an Avatar', () => {
     const wrapper = shallow((
       <ListItemAvatar>
-        <span />
+        <Avatar />
       </ListItemAvatar>
     ), {
       context: {
         dense: true,
       },
     });
-    assert.strictEqual(wrapper.is('span'), true, 'should be a span');
+    assert.strictEqual(wrapper.is('Avatar'), true, 'should be an Avatar');
   });
 
   it('should render with the user and root classes', () => {
     const wrapper = shallow((
       <ListItemAvatar className="foo">
-        <span className="bar" />
+        <Avatar className="bar" />
       </ListItemAvatar>
     ), {
       context: {
@@ -40,6 +41,7 @@ describe('<ListItemAvatar />', () => {
     });
     assert.strictEqual(wrapper.hasClass('foo'), true, 'should have the "foo" class');
     assert.strictEqual(wrapper.hasClass('bar'), true, 'should have the "bar" class');
-    assert.strictEqual(wrapper.hasClass(classes.dense), true, 'should have the dense class');
+    assert.strictEqual(wrapper.hasClass(classes.denseAvatar), true,
+      'should have the denseAvatar class');
   });
 });
diff --git a/src/styles/MuiThemeProvider.js b/src/styles/MuiThemeProvider.js
@@ -60,6 +60,8 @@ export const MUI_SHEET_ORDER = [
   'MuiAppBar',
   'MuiDrawer',
 
+  'MuiAvatar',
+
   'MuiListItem',
   'MuiListItemText',
   'MuiListItemSecondaryAction',
@@ -71,8 +73,6 @@ export const MUI_SHEET_ORDER = [
   'MuiMenu',
   'MuiMenuItem',
 
-  'MuiAvatar',
-
   'MuiCardContent',
   'MuiCardMedia',
   'MuiCardActions',