Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[List] Making list meet Material Guidelines #6316

Closed
wants to merge 20 commits into from
Closed

[List] Making list meet Material Guidelines #6316

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
4be924d
Single line list item adjustments to meet Mterial Guidelines
kybarg Mar 9, 2017
ed0e996
Fixed lint
kybarg Mar 9, 2017
40b90a0
More fancy demo jagged edges
kybarg Mar 9, 2017
e8277e2
Simplified demo styles
kybarg Mar 9, 2017
a852b81
Responsive demo
kybarg Mar 9, 2017
2eed341
Better single-line list demo
kybarg Mar 9, 2017
510f6f2
Fixed sibgle-line list dense text size
kybarg Mar 9, 2017
8a3da54
Moved "dense" param to context
kybarg Mar 9, 2017
21f4cd6
Use CSS inheritance instead instead of class name
kybarg Mar 9, 2017
7328c84
Using `some` to detec child type
kybarg Mar 9, 2017
26428d4
Fixed avatar detection
kybarg Mar 9, 2017
06846b5
Dense property fix
kybarg Mar 9, 2017
2af14ec
Demo fixes
kybarg Mar 9, 2017
aee5667
Merge branch 'callemall/next' into next-list-guidelines
kybarg Mar 9, 2017
6b137ae
Merge branch 'callemall/next' into next-list-guidelines
kybarg Mar 15, 2017
33d1f41
Merge branch 'callemall/next' into next-list-guidelines
kybarg Mar 15, 2017
c7c3f1d
Fixes
kybarg Mar 15, 2017
824709c
Svg background in example
kybarg Mar 15, 2017
87e4f6d
Revert "Svg background in example"
kybarg Mar 17, 2017
ba004b1
Two-lines list example
kybarg Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
209 changes: 209 additions & 0 deletions docs/site/src/demos/lists/SingleLineList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
// @flow weak

import React, { Component } from 'react';
import { createStyleSheet } from 'jss-theme-reactor';
import customPropTypes from 'material-ui/utils/customPropTypes';
import {
List,
ListItem,
ListItemText,
ListItemIcon,
ListItemSecondaryAction,
} from 'material-ui/List';
import Avatar from 'material-ui/Avatar';
import FolderIcon from 'material-ui/svg-icons/folder';
import IconButton from 'material-ui/IconButton';
import { LabelCheckbox } from 'material-ui/Checkbox';
import Layout from 'material-ui/Layout';
import SvgIcon from 'material-ui/SvgIcon';
import Text from 'material-ui/Text';

const SqureIcon = (props) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mbrookes This one is for you 💃

<SvgIcon {...props} viewBox="3 3 18 18">
<path d="M3,3V21H21V3" />
</SvgIcon>
);

const styleSheet = createStyleSheet('SingleLineList', (theme) => ({
root: {
flexGrow: 1,
maxWidth: 752,
},
container: {
width: '100%',
},
demoContainer: {
margin: '0 auto',
maxWidth: 360,
},
demo: {
background: theme.palette.background.paper,
border: `solid ${theme.palette.text.divider}`,
borderWidth: '1px 1px 0',
marginBottom: 16,
maxHeight: 160,
overflow: 'hidden',
position: 'relative',
width: '100%',
'&:before': {
backgroundImage: `linear-gradient(45deg, ${theme.palette.background.contentFrame} 25%, transparent 25%),
linear-gradient(-45deg, ${theme.palette.background.contentFrame} 25%, transparent 25%)`,
backgroundSize: '20px 20px',
backgroundPosition: '10px -10px',
bottom: 0,
content: '""',
Copy link
Member

@oliviertassinari oliviertassinari Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

display: 'block',
height: 10,
position: 'absolute',
width: '100%',
zIndex: 99,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using that value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@oliviertassinari oliviertassinari Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

},
},
title: {
margin: '32px 0 16px',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`,

},
}));

class SingleLineList extends Component {
static contextTypes = {
styleManager: customPropTypes.muiRequired,
};

state = {
dense: false,
};

render() {
const classes = this.context.styleManager.render(styleSheet);
const { dense } = this.state

return (
<div className={classes.root}>
<LabelCheckbox
checked={dense}
onChange={(event, checked) => this.setState({ dense: checked })}
label="Enable dense preview"
value="dense"
/>
<Layout container className={classes.container}>
<Layout item xs={12} md={6}>
<div className={classes.demoContainer}>
<Text type="title" className={classes.title}>Text only</Text>
<div className={classes.demo}>
<List dense={dense}>
<ListItem button>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemText primary="Single-line item" />
</ListItem>
</List>
</div>
<Text type="body1" secondary>Single-line list</Text>
</div>
</Layout>
<Layout item xs={12} md={6}>
<div className={classes.demoContainer}>
<Text type="title" className={classes.title}>Icon with text</Text>
<div className={classes.demo}>
<List dense={dense}>
<ListItem button>
<ListItemIcon><SqureIcon /></ListItemIcon>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemIcon><SqureIcon /></ListItemIcon>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemIcon><SqureIcon /></ListItemIcon>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<ListItemIcon><SqureIcon /></ListItemIcon>
<ListItemText primary="Single-line item" />
</ListItem>
</List>
</div>
<Text type="body1" secondary>Single-line list with icon</Text>
</div>
</Layout>
</Layout>
<Layout container className={classes.container}>
<Layout item xs={12} md={6}>
<div className={classes.demoContainer}>
<Text type="title" className={classes.title}>Avatar with text</Text>
<div className={classes.demo} style={{ maxHeight: 184 }}>
<List dense={dense}>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
</ListItem>
</List>
</div>
<Text type="body1" secondary>Single-line item with avatar</Text>
</div>
</Layout>
<Layout item xs={12} md={6}>
<div className={classes.demoContainer}>
<Text type="title" className={classes.title}>Avatar with text and icon</Text>
<div className={classes.demo} style={{ maxHeight: 184 }}>
<List dense={dense}>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
<ListItemSecondaryAction>
<IconButton>comment</IconButton>
</ListItemSecondaryAction>
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
<ListItemSecondaryAction>
<IconButton>comment</IconButton>
</ListItemSecondaryAction>
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
<ListItemSecondaryAction>
<IconButton>comment</IconButton>
</ListItemSecondaryAction>
</ListItem>
<ListItem button>
<Avatar><FolderIcon /></Avatar>
<ListItemText primary="Single-line item" />
<ListItemSecondaryAction>
<IconButton>comment</IconButton>
</ListItemSecondaryAction>
</ListItem>
</List>
</div>
<Text type="body1" secondary>Single-line item with avatar and icon</Text>
</div>
</Layout>
</Layout>
</div>
);
}
}

export default SingleLineList;
4 changes: 4 additions & 0 deletions docs/site/src/demos/lists/lists.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ Lists are made up of a continuous column of rows. Each row contains a tile. Prim

Lists are best suited for similar data types.

### Single-line list

{{demo='demos/lists/SingleLineList.js'}}

### Simple List

{{demo='demos/lists/SimpleList.js'}}
Expand Down
12 changes: 11 additions & 1 deletion src/List/List.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ export const styleSheet = createStyleSheet('MuiList', () => {
paddingTop: 8,
paddingBottom: 8,
},
dense: {
paddingTop: 4,
paddingBottom: 4,
},
subheader: {
paddingTop: 0,
},
Expand All @@ -33,20 +37,24 @@ export default function List(props, context) {
component: ComponentProp,
padding,
children,
dense,
subheader,
rootRef,
...other
} = props;
const classes = context.styleManager.render(styleSheet);
const className = classNames(classes.root, {
[classes.dense]: dense,
[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 }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)}
</ComponentProp>
);
}
Expand All @@ -65,6 +73,7 @@ List.propTypes = {
PropTypes.string,
PropTypes.func,
]),
dense: PropTypes.bool,
padding: PropTypes.bool,
/**
* @ignore
Expand All @@ -75,6 +84,7 @@ List.propTypes = {

List.defaultProps = {
component: 'div',
dense: false,
padding: true,
};

Expand Down
34 changes: 27 additions & 7 deletions src/List/ListItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,18 @@ export const styleSheet = createStyleSheet('MuiListItem', (theme) => {
background: palette.text.divider,
},
default: {
paddingTop: 19,
paddingBottom: 19,
paddingTop: 12,
paddingBottom: 12,
},
dense: {
paddingTop: 8,
paddingBottom: 8,
},
avatarDense: {
height: '32px !important',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!important sounds definitely like a hack.

marginRight: 8,
width: '32px !important',
},
disabled: {
opacity: 0.5,
},
Expand All @@ -54,6 +59,14 @@ export const styleSheet = createStyleSheet('MuiListItem', (theme) => {
};
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@kybarg kybarg Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const props = {};
if (child.type.name === 'ListItemIcon' || child.type.name === 'ListItemText') props.dense = dense;
if (child.type.name === 'Avatar' && dense) props.className = classes.avatarDense;

return React.cloneElement(child, props);
});

export default class ListItem extends Component {
static propTypes = {
button: PropTypes.bool,
Expand Down Expand Up @@ -106,12 +119,21 @@ export default class ListItem extends Component {
} = this.props;

const classes = this.context.styleManager.render(styleSheet);
const children = React.Children.toArray(childrenProp);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or even better use some.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That perform better

// if (child.type.name === 'ListItemIcon') hasIcon = true;
if (child.type.name === 'Avatar') hasAvatar = true;
});

const className = classNames(classes.listItem, {
[classes.gutters]: gutters,
[classes.divider]: divider,
[classes.disabled]: disabled,
[classes.button]: button,
[dense ? classes.dense : classes.default]: true,
[dense || hasAvatar ? classes.dense : classes.default]: true,
}, classNameProp);

const listItemProps = { className, disabled, ...other };
Expand All @@ -123,8 +145,6 @@ export default class ListItem extends Component {
listItemProps.keyboardFocusedClassName = classes.keyboardFocused;
}

const children = React.Children.toArray(childrenProp);

if (
children.length &&
children[children.length - 1].type &&
Expand All @@ -134,7 +154,7 @@ export default class ListItem extends Component {
return (
<div className={classes.listItemContainer}>
<ComponentMain {...listItemProps}>
{children}
{mapListIemChildren(children, classes, dense)}
</ComponentMain>
{secondaryAction}
</div>
Expand All @@ -143,7 +163,7 @@ export default class ListItem extends Component {

return (
<ComponentMain {...listItemProps}>
{children}
{mapListIemChildren(children, classes, dense)}
</ComponentMain>
);
}
Expand Down
Loading