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.

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/ListItem] Introduce selected property #1581 #1976

Merged
merged 2 commits into from
Nov 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
174 changes: 173 additions & 1 deletion docs/src/app/components/pages/components/lists.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const ContentSend = require('svg-icons/content/send');
const EditorInsertChart = require('svg-icons/editor/insert-chart');
const FileFolder = require('svg-icons/file/folder');
const MoreVertIcon = require('svg-icons/navigation/more-vert');
import { SelectableContainerEnhance } from 'material-ui/hoc/selectable-enhance';

const {
Avatar,
Expand All @@ -34,11 +35,66 @@ const { Colors } = Styles;
const Code = require('lists-code');
const CodeExample = require('../../code-example/code-example');
const CodeBlock = require('../../code-example/code-block');
let SelectableList = SelectableContainerEnhance(List);

const Typography = Styles.Typography;
let styles = {
headline: {
fontSize: '24px',
lineHeight: '32px',
paddingTop: '16px',
marginBottom: '12px',
letterSpacing: '0',
fontWeight: Typography.fontWeightNormal,
color: Typography.textDarkBlack,
},
subheadline: {
fontSize: '18px',
lineHeight: '27px',
paddingTop: '12px',
marginBottom: '9px',
letterSpacing: '0',
fontWeight: Typography.fontWeightNormal,
color: Typography.textDarkBlack,
},
codeblock: {
padding: '24px',
marginBottom: '32px',
},
}

function wrapState(ComposedComponent) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we could avoid having this helper function to wrap SelectableList? And do it straight using props in the render method of ListsPage? I think it would be a good idea to keep the docs code as straightforward as possible..

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it is avoidable, but it makes the <ListsPage> really slow. There is a noticable lag if you click on an item of the <SelectableList>. See the performance-Section of my previous comment.

const StateWrapper = React.createClass({
getInitialState() {
return { selectedIndex: 1 };
},
handleUpdateSelectedIndex(e, index) {
this.setState({
selectedIndex: index,
});
},
render() {
return <ComposedComponent {...this.props} {...this.state}
valueLink={{value: this.state.selectedIndex, requestChange: this.handleUpdateSelectedIndex}} />;
},
});
return StateWrapper;
}

SelectableList = wrapState(SelectableList);


export default class ListsPage extends React.Component {

constructor(props) {
super(props);
this.state = { selectedIndex: 1 }

this.handleUpdateSelectedIndex = (e, index) => {
this.setState({
selectedIndex: index,
});
}
}

render() {
Expand All @@ -53,6 +109,12 @@ export default class ListsPage extends React.Component {
header: 'default: false',
desc: 'If true, the subheader will be indented by 72px.',
},
{
name: 'selectedItemStyle',
type: 'object',
header: 'optional, only available if HOC SelectableContainerEnhance is used',
desc: 'Override the choosen inline-styles to indicate a <ListItem> is highlighted. You can set e.g. the background color here like this way: {{backgroundColor: #da4e49}}.',
},
{
name: 'style',
type: 'object',
Expand All @@ -71,6 +133,13 @@ export default class ListsPage extends React.Component {
header: 'optional',
desc: 'The style object to override subheader styles.',
},
{
name: 'valueLink',
type: 'valueLink',
header: 'optional, only available if HOC SelectableContainerEnhance is used',
desc: 'Makes List controllable. Highlights the ListItem whose index prop matches this "selectedLink.value". ' +
'"selectedLink.requestChange" represents a callback function to change that value (e.g. in state).',
},
],
},
{
Expand Down Expand Up @@ -182,6 +251,13 @@ export default class ListsPage extends React.Component {
header: 'optional',
desc: 'Override the inline-styles of the list item\'s root element.',
},
{
name: 'value',
type: 'number',
header: 'optional, only available if HOC SelectableContainerEnhance is used',
desc: 'If valueLink prop is passed to List component, this prop is also required. It assigns an identifier ' +
'to the listItem so that it can be hightlighted by the List.',
},
],
},
{
Expand Down Expand Up @@ -666,9 +742,105 @@ export default class ListsPage extends React.Component {
secondaryTextLines={2} />
</List>
</MobileTearSheet>
<MobileTearSheet>
<SelectableList
value={3}
subheader="SelectableContacts">

<ListItem
value={1}
primaryText="Brendan Lim"
leftAvatar={<Avatar src="images/ok-128.jpg" />} />
<ListItem value={2}
primaryText="Grace Ng"
leftAvatar={<Avatar src="images/uxceo-128.jpg" />} />
<ListItem value={3}
primaryText="Kerem Suer"
leftAvatar={<Avatar src="images/kerem-128.jpg" />} />
<ListItem value={4}
primaryText="Eric Hoffman"
leftAvatar={<Avatar src="images/kolage-128.jpg" />} />
<ListItem value={5}
primaryText="Raquel Parrado"
leftAvatar={<Avatar src="images/raquelromanp-128.jpg" />} />
</SelectableList>
</MobileTearSheet>
</CodeExample>

<Paper style={{padding: '24px', marginBottom: '32px'}}>
<div>
<h2 style={styles.headline}>Selectable Lists</h2>
<p>
Basically three steps are needed:
</p>
<ul>
<li>enhance <code>&lt;List&gt;</code> with HOC</li>
<li>decide where to put state</li>
<li>implement and set valueLink</li>
</ul>


<h3 style={styles.subheadline}> Enhance List</h3>
<p>
Wrapping the <code>&lt;List&gt;</code> component with the higher order component "SelectableEnhance" enables
the clicked <code>&lt;ListItem&gt;</code> to be highlighted.
</p>
<div style={styles.codeblock}>
<CodeBlock>
{`import { SelectableContainerEnhance } from 'material-ui/lib/hoc/selectable-enhance';
.
.
.
var SelectableList = SelectableContainerEnhance(List);
`}
</CodeBlock>
</div>


<h3 style={styles.subheadline}>Where to put state</h3>
<p>
If this component is used in conjunction with flux or redux this is a no-brainer. The callback-handler
just has to update the store. Otherwise the state can be held within e.g the parent, but it is to be to
considered that each time a <code>&lt;ListItem&gt;</code> is clicked, the state will update and the parent - including it's
children - will rerender.
</p>
<p>
A possible solution for this is to use another hoc. An example can be found in the sourcecode
of <code>docs/src/app/components/pages/components/lists.jsx</code>.
</p>
<h3 style={styles.subheadline}>The valueLink</h3>
<p>
The prop 'valueLink' of <code>&lt;List&gt;</code> has to be set, to make the highlighting controllable:
</p>
<div style={styles.codeblock}>
<CodeBlock>
{`valueLink={{
value: this.state.selectedIndex,
requestChange: this.handleUpdateSelectedIndex}}
`}
</CodeBlock>
</div>
A sample implementation might look like this.
<div style={styles.codeblock}>
<CodeBlock>
{`getInitialState() {
return { selectedIndex: 1 };
},
handleUpdateSelectedIndex(e,index) {
this.setState({
selectedIndex: index,
});
`}
</CodeBlock>
</div>
<h3 style={styles.subheadline}>Adjust the <code>&lt;ListItem&gt;</code></h3>
<p>
The prop "value" on each ListItem has to be set. This makes the item addressable for the callback.
</p>
</div>
</Paper>
</ComponentDoc>
);
}

}
}
18 changes: 18 additions & 0 deletions docs/src/app/components/raw-code/lists-code.txt
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,21 @@
]}
/>
</List>

// List with selected indicator based on HOC
<SelectableList
valueLink={{value: this.state.selectedIndex, requestChange: this.handleUpdateSelectedIndex}}
subheader="Contacts">
<ListItem
value={1}
primaryText="Brendan Lim"
leftAvatar={<Avatar src="images/ok-128.jpg" />}/>
<ListItem
value={2}
primaryText="Grace Ng"
leftAvatar={<Avatar src="images/uxceo-128.jpg" />} />
<ListItem
value={3}
primaryText="Kerem Suer"
leftAvatar={<Avatar src="images/kerem-128.jpg" />} />
</SelectableList>
2 changes: 1 addition & 1 deletion docs/webpack-dev-server.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ var config = {
loader:'babel-loader?optional=runtime&stage=0', //react-hot is like browser sync and babel loads jsx and es6-7
include: [__dirname, path.resolve(__dirname, '../src')], //include these files
exclude: [nodeModulesPath] //exclude node_modules so that they are not all compiled
}
},
]
},
eslint: {
Expand Down
124 changes: 124 additions & 0 deletions src/hoc/selectable-enhance.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
const React = require('react');
const ThemeManager = require('../styles/theme-manager');
const StylePropable = require('../mixins/style-propable');
const ColorManipulator = require('../utils/color-manipulator');
const DefaultRawTheme = require('../styles/raw-themes/light-raw-theme');

export const SelectableContainerEnhance = (Component) => {
let composed = React.createClass({

mixins: [StylePropable],

contextTypes: {
muiTheme: React.PropTypes.object,
},

displayName: `Selectable${Component.displayName}`,

propTypes: {
valueLink: React.PropTypes.shape({
value: React.PropTypes.number,
requestChange: React.PropTypes.func,
}).isRequired,
selectedItemStyle: React.PropTypes.object,
},

childContextTypes: {
muiTheme: React.PropTypes.object,
},

getChildContext () {
return {
muiTheme: this.state.muiTheme,
};
},

componentWillReceiveProps (nextProps, nextContext) {
let newMuiTheme = nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme;
this.setState({muiTheme: newMuiTheme});
},

getInitialState() {
return {
muiTheme: this.context.muiTheme ? this.context.muiTheme : ThemeManager.getMuiTheme(DefaultRawTheme),
};
},

getValueLink: function(props) {
return props.valueLink || {
value: props.value,
requestChange: props.onChange,
};
},

render(){
const { children, selectedItemStyle } = this.props
let listItems;
let keyIndex = 0;
let styles = {};

if (! selectedItemStyle) {
let textColor = this.state.muiTheme.rawTheme.palette.textColor;
let selectedColor = ColorManipulator.fade(textColor, 0.2);
styles = {
backgroundColor: selectedColor,
};
}

listItems = React.Children.map(children, (child) => {
if (child.type.displayName === "ListItem") {
let selected = this._isChildSelected(child, this.props);
let selectedChildrenStyles = {};
if (selected) {
selectedChildrenStyles = this.mergeStyles(styles, selectedItemStyle);
}

let mergedChildrenStyles = this.mergeStyles(
child.props.style || {},
selectedChildrenStyles
);

keyIndex += 1;

return React.cloneElement(child, {
onTouchTap: (e) => {
this._handleItemTouchTap(e, child);
if (child.props.onTouchTap) { child.props.onTouchTap(e) };
},
key: keyIndex,
Copy link
Member

Choose a reason for hiding this comment

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

key and ref from the original element will be preserved

From the React doc. Shouldn't we remove this?

Copy link
Author

Choose a reason for hiding this comment

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

@oliviertassinari Is there any benefit for the user of the component to let him set the key which I do not see? Otherwise I would like to mitigate the "boilerplate" and source of error/warnings here.

Copy link
Member

Choose a reason for hiding this comment

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

You are right.
Second question then. Do we need a key at all here? For an array we do, but since you are using React.Children.map I'm not sure.

Copy link
Author

Choose a reason for hiding this comment

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

@oliviertassinari Quite frankly, I didn't know. React.Children.map does iterate over children and returns an array, thats for sure.
But adding the key anyway has the benefit, that for the usecase "<ListItems> generated by loop" the user has not to remember adding the key.
Somehow in the codebase of <ListsPage>this problem already exist. If you run the doc in development env, you get this nasty Warning: Each child in an array or iterator should have a unique "key" prop. (I couldn't track down the reason of this, but if I find, I will create an issue and submit a PR :-) )

Copy link
Member

Choose a reason for hiding this comment

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

@frankf-cgn Let's keep the key then. Can you have a look at my comment on the import? I think that we can merge then 👍.

style: mergedChildrenStyles,
});
}
else {
return child;
}
});
let newChildren = listItems;

return (
<Component {...this.props} {...this.state}>
{newChildren}
</Component>
);
},

_isChildSelected(child, props) {
let itemValue = this.getValueLink(props).value;
let childValue = child.props.value;

return (itemValue && itemValue === childValue);
},

_handleItemTouchTap(e, item) {
let valueLink = this.getValueLink(this.props);
let itemValue = item.props.value;
let menuValue = valueLink.value
if ( itemValue !== menuValue) {
valueLink.requestChange(e, itemValue);
}
},

});
return( composed );
};

1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ module.exports = {
RefreshIndicator: require('./refresh-indicator'),
Ripples: require('./ripples/'),
SelectField: require('./select-field'),
SelectableContainerEnhance: require('./hoc/selectable-enhance'),
Slider: require('./slider'),
SvgIcon: require('./svg-icon'),
Icons: {
Expand Down