[List] Making list meet Material Guidelines #6316

kybarg · 2017-03-09T04:41:33Z

Trying to make list component more functional following Material Design Guidelines

(Cheking if functionality is implemented and demo is ready)

Fancy demo:

oliviertassinari

Some early feedback. That's looks really good 😍 .
My main concern for now would be with the performance implication of iterating throw the children.
Once that's sorted, we would definitely needs to add some visual regression tests.
Thanks for working in that 👍

oliviertassinari · 2017-03-09T12:51:12Z

src/List/List.js

    [classes.padding]: padding,
    [classes.subheader]: subheader,
  }, classNameProp);

  return (
    <ComponentProp ref={rootRef} className={className} {...other}>
      {subheader}
-      {children}
+      {React.Children.map(children, (child) =>
+        React.cloneElement(child, { dense }),


I would rather be using the context. I think that it would perform much better.
Eventually, we would need a perf benchmark.

oliviertassinari · 2017-03-09T12:51:32Z

src/List/ListItem.js

    },
    dense: {
      paddingTop: 8,
      paddingBottom: 8,
    },
+    avatarDense: {
+      height: '32px !important',


!important sounds definitely like a hack.

oliviertassinari · 2017-03-09T12:52:36Z

src/List/ListItemText.js

  return (
    <div className={className} {...other}>
      {primary && (
        typeof primary === 'string' ? (
-          <Text type="subheading">
+          <Text type="subheading" className={classNameText}>


What about relying on CSS inheritance instead?

oliviertassinari · 2017-03-09T12:52:44Z

src/List/ListItemText.js

@@ -20,12 +20,16 @@ export const styleSheet = createStyleSheet('MuiListItemText', () => {
        paddingLeft: 56,
      },
    },
+    dense: {
+      fontSize: '13px',


fontSize: 13,

oliviertassinari · 2017-03-09T12:56:16Z

src/List/ListItem.js

+
+    // let hasIcon;
+    let hasAvatar;
+    React.Children.map(children, (child) => {


use forEach on the children instead, you are paying the Children method cost twice.

Or even better use some.

That perform better

oliviertassinari · 2017-03-09T12:59:16Z

src/List/ListItem.js

@@ -54,6 +59,14 @@ export const styleSheet = createStyleSheet('MuiListItem', (theme) => {
  };
 });

+const mapListIemChildren = (children, classes, dense) => React.Children.map(children, (child) => {


If we have to perform such expensive operation/brittle logic, I think that relying on the context would be better.
(We would need a ListItemAvar component if we follow that direction)

@oliviertassinari Definitely context will rock here ☺️ my lack of experience is making me find hard ways in first place 😂

oliviertassinari · 2017-03-09T22:38:47Z

docs/site/src/demos/lists/SingleLineList.js

+import SvgIcon from 'material-ui/SvgIcon';
+import Text from 'material-ui/Text';
+
+const SqureIcon = (props) => (


@mbrookes This one is for you 💃

oliviertassinari · 2017-03-09T22:39:31Z

docs/site/src/demos/lists/SingleLineList.js

+      backgroundSize: '20px 20px',
+      backgroundPosition: '10px -10px',
+      bottom: 0,
+      content: '""',


Good, doing the other way around crash on IE11 "''" cssinjs/jss#242

@oliviertassinari So content: '""' or content: "''" is the right way? 😄

oliviertassinari

12 problems (3 errors, 9 warnings)

You have the following eslint issues.

What's your plan going forward? I would avoid doing something too ambitious. We would definitely needs adding visual regressions tests for the new use cases.

oliviertassinari · 2017-03-09T22:40:28Z

docs/site/src/demos/lists/SingleLineList.js

+      height: 10,
+      position: 'absolute',
+      width: '100%',
+      zIndex: 99,


Why using that value?

@oliviertassinari You mean z-index? Basically to be over all elements in demo but not overlap toolbar when scrolling.

In that case, you could be using the zIndex key of the theme. That would add constraints to the value. (Less likely to break in the future).

oliviertassinari · 2017-03-09T22:41:45Z

src/List/ListItem.js

@@ -104,14 +105,21 @@ export default class ListItem extends Component {
      gutters,
      ...other
    } = this.props;
-
+    const isDense = dense || this.context.dense;


const isDense = dense || this.context.dense || false;

To be safe?

oliviertassinari · 2017-03-09T22:43:33Z

src/List/ListItem.js

+    const children = React.Children.toArray(childrenProp);
+
+    let hasAvatar;
+    Object.entries(children).some((value) => {


That looks suspicious. What about

const hasAvatar = children.some((child) => {

oliviertassinari · 2017-03-09T22:44:38Z

src/List/ListItemAvatar.js

+      className: classNameProp,
+      ...other
+    } = this.props;
+    const { dense } = this.context;


Looks overkill to destructure.

oliviertassinari · 2017-03-09T22:45:05Z

src/List/ListItemAvatar.js

+    const { dense } = this.context;
+    const classes = this.context.styleManager.render(styleSheet);
+    const className = classNames(classes.root, {
+      [classes.dense]: dense,


What about?

[classes.dense]: this.context.dense || false,

oliviertassinari · 2017-03-09T22:46:55Z

src/List/ListItemText.js

@@ -20,6 +20,11 @@ export const styleSheet = createStyleSheet('MuiListItemText', () => {
        paddingLeft: 56,
      },
    },
+    dense: {
+      '& > *': {


I was thinking about a simple CSS inheritance. Do we need to target the children elements?
I think that it would be better that the fontSize naturally inherent. Maybe we can't. I haven't looked in detail. That's always better than before 👍 .

<Text /> component is a child and it has fontSize style already. SO just using fontSize on parent has no effect.

We have a colorInherit property on the Text component. We could imagine a fontSizeInherit property.

@octavioamu would be nice

So, what do you think about @mbrookes's idea.

We set the font size to the parent element

We add a class on the Text to get the inheritance.

The win is a flatten CSS selector, that's always better for composition.

oliviertassinari · 2017-03-13T23:03:30Z

@kybarg I have lost track of the state of this PR. Do you need any feedback?

mbrookes · 2017-03-14T01:12:51Z

Yay for reincarnation of the MobileTearSheet! 🎉 ❤️

I have a small hack for continuing the border along the zig-zig that I'll post tomorrow (not on the right machine at the mo).

Also, I appreciate you were trying to recreate the MDM guidelines, but a real icon might be preferable to the placeholder square for 'Icon with text".

kybarg · 2017-03-14T05:24:20Z

@oliviertassinari Yes, if everything is fine with my current fixes, I can continue with two and three line variations. Also we haven't agreed on how to change text size for dense list.

@mbrookes Tried to add border to jugged edge but it looks ugly in Firefox and IE 😄 They render it differently and it looks like 2px border. I hope you can do it better then me ☺️

mbrookes · 2017-03-14T22:04:58Z

@kybarg Grrr, no, you're right! 😞 I get the same 2px in Safari. I had only tried in Chrome. We could use an SVG instead. :)

Otherwise, still to do:

Replace the Square icon and comment icon with an SvgIcon, (Or, if we're going the other way around, and showing specification not examples, replace the Avatar with a circle / empty Avatar, and comment icon with a square, and change the color to dark gray, not black.)
Agree styling approach for text size (@oliviertassinari?)

oliviertassinari

I have made minor comments, it's 80% ready to be merged 😄 . You rock 🚀
Also, we gonna need some visual regression tests for that new dense property.

oliviertassinari · 2017-03-14T22:28:13Z

docs/site/src/demos/lists/SingleLineList.js

+    },
+  },
+  title: {
+    margin: '32px 0 16px',


I would rathe be using the unit to show it's existing and that spacing should be consitent.

margin: `${theme.spacing.unit * 4}px 0 ${theme.spacing.unit * 2}px`,

oliviertassinari · 2017-03-14T22:34:42Z

src/List/ListItemText.js

@@ -20,6 +20,11 @@ export const styleSheet = createStyleSheet('MuiListItemText', () => {
        paddingLeft: 56,
      },
    },
+    dense: {
+      '& > *': {


So, what do you think about @mbrookes's idea.

We set the font size to the parent element

We add a class on the Text to get the inheritance.

The win is a flatten CSS selector, that's always better for composition.

1. Using spacing unit for margins 2. Fixed icon in example 3. Simplified `ListItemAvatar` logic 4. Proper `Text` font size inheritance 5. Removed tests for `ListItemAvatar` (need to agree on final implementation)

oliviertassinari · 2017-03-15T20:19:12Z

docs/site/src/demos/lists/SingleLineList.js

+import { LabelCheckbox } from 'material-ui/Checkbox';
+import Layout from 'material-ui/Layout';
+import Text from 'material-ui/Text';
+import TriangleImageLight from 'docs/site/assets/images/bg-triangle-light.svg';


Oh noooo, the demo is no longer working in isolation. It's going to be a pain for user starting from the demos.
@mbrookes What do you suggest?

@oliviertassinari Either the <svg> could be defined inline rather than being imported, or the file be treated as an img, so will fail gracefully if the example code is copied without the svg file.

@kybarg Could the CSS fill & stroke attributes perhaps be used so as to not need two separate svgs?

You see me coming but simplicity might be just as good: removing that fancy effect.
I hope we will find a better solution.

Nooooo!! 🤣 🤣 🤣

oliviertassinari · 2017-03-15T20:20:03Z

src/List/ListItemAvatar.js

+  };
+});
+
+export default class ListItemAvatar extends Component {


No need for a Component here.

oliviertassinari · 2017-03-15T20:21:54Z

@kybarg The build fails because of the regressions tests. The changes look fine to me. I can take care of writing new one for the dense property and updating the base screenshots if needed.

kybarg · 2017-03-15T20:56:57Z

@oliviertassinari would be great! 👍

This reverts commit 824709c.

oliviertassinari · 2017-03-17T18:54:01Z

@kybarg Hold on. I'm continuing the PR to add the missing tests 😄 .

oliviertassinari · 2017-03-17T21:11:44Z

Here is it #6367. Thanks for pushing it forward!

Single line list item adjustments to meet Mterial Guidelines

4be924d

muibot added the PR: Needs Review label Mar 9, 2017

kybarg changed the title ~~[List] Making list meet Mterial Guidelines~~ [List] Making list meet Material Guidelines Mar 9, 2017

muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 9, 2017

Fixed lint

ed0e996

muibot added the PR: Needs Review label Mar 9, 2017

kybarg added 5 commits March 9, 2017 08:58

More fancy demo jagged edges

40b90a0

Simplified demo styles

e8277e2

Responsive demo

a852b81

Better single-line list demo

2eed341

Fixed sibgle-line list dense text size

510f6f2

oliviertassinari reviewed Mar 9, 2017

View reviewed changes

oliviertassinari added component: list This is the name of the generic UI component, not the React module! next and removed PR: Needs Review labels Mar 9, 2017

Moved "dense" param to context

8a3da54

muibot added the PR: Needs Review label Mar 9, 2017

kybarg added 2 commits March 9, 2017 17:25

Use CSS inheritance instead instead of class name

21f4cd6

Using some to detec child type

7328c84

oliviertassinari reviewed Mar 9, 2017

View reviewed changes

oliviertassinari requested changes Mar 9, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Mar 9, 2017

kybarg added 2 commits March 10, 2017 01:04

Fixed avatar detection

26428d4

Dense property fix

06846b5

muibot added the PR: Needs Review label Mar 9, 2017

kybarg added 2 commits March 10, 2017 01:31

Demo fixes

2af14ec

Merge branch 'callemall/next' into next-list-guidelines

aee5667

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 11, 2017

oliviertassinari requested changes Mar 14, 2017

View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 14, 2017

kybarg added 3 commits March 15, 2017 09:24

Merge branch 'callemall/next' into next-list-guidelines

6b137ae

Merge branch 'callemall/next' into next-list-guidelines

33d1f41

Fixes

c7c3f1d

1. Using spacing unit for margins 2. Fixed icon in example 3. Simplified `ListItemAvatar` logic 4. Proper `Text` font size inheritance 5. Removed tests for `ListItemAvatar` (need to agree on final implementation)

muibot added the PR: Needs Review label Mar 15, 2017

Svg background in example

824709c

oliviertassinari reviewed Mar 15, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 15, 2017

kybarg added 2 commits March 17, 2017 16:56

Revert "Svg background in example"

87e4f6d

This reverts commit 824709c.

Two-lines list example

ba004b1

muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 17, 2017

oliviertassinari closed this Mar 17, 2017

oliviertassinari mentioned this pull request Mar 17, 2017

[List] Finish the initial PR #6367

Merged

3 tasks

kybarg deleted the next-list-guidelines branch April 10, 2017 03:05

[List] Making list meet Material Guidelines #6316

[List] Making list meet Material Guidelines #6316

Conversation

kybarg commented Mar 9, 2017 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kybarg Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

kybarg Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented Mar 13, 2017

mbrookes commented Mar 14, 2017

kybarg commented Mar 14, 2017 • edited Loading

mbrookes commented Mar 14, 2017

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Mar 15, 2017

kybarg commented Mar 15, 2017 • edited Loading

oliviertassinari commented Mar 17, 2017

oliviertassinari commented Mar 17, 2017 • edited Loading

kybarg commented Mar 9, 2017 •

edited

Loading

kybarg Mar 9, 2017 •

edited

Loading

oliviertassinari Mar 9, 2017 •

edited

Loading

oliviertassinari Mar 9, 2017 •

edited

Loading

oliviertassinari Mar 9, 2017 •

edited

Loading

kybarg Mar 9, 2017 •

edited

Loading

kybarg commented Mar 14, 2017 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari Mar 15, 2017 •

edited

Loading

oliviertassinari Mar 15, 2017 •

edited

Loading

kybarg commented Mar 15, 2017 •

edited

Loading

oliviertassinari commented Mar 17, 2017 •

edited

Loading