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

[SelectField] [DropDownMenu] Support multi select #6165

Merged
merged 22 commits into from
Mar 11, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
91e080f
[SelectField] Add multi select to SelectField and DropDownMenu
Feb 14, 2017
265dc8c
[SelectField] Add onChangeRenderer
JessThrysoee Feb 19, 2017
7c0082d
rename onChangeRenderer to selectionRenderer
JessThrysoee Feb 20, 2017
715a557
Add ExampleMultiSelect docs example
JessThrysoee Feb 20, 2017
b055791
Handle multiple value initialized to null
JessThrysoee Feb 20, 2017
725c4dc
Adhere to naming convension: handleOnChange -> handleChange
JessThrysoee Feb 21, 2017
2a181b3
Fix lint errors
JessThrysoee Feb 21, 2017
e18f403
Handle initial null menuValue
JessThrysoee Feb 24, 2017
af05629
Add multi select test to Menu
JessThrysoee Feb 23, 2017
ada7f96
Add multi select test to DropDownMenu
JessThrysoee Feb 23, 2017
57e574b
Add multi select test to SelectField
JessThrysoee Feb 24, 2017
5df3184
Always pass value when a selectionRenderer is provided.
JessThrysoee Mar 2, 2017
564125f
Remove blank lines from render
JessThrysoee Mar 6, 2017
697deb4
Avoid function indirection and destructuration overkill
JessThrysoee Mar 6, 2017
b738e6e
Assert state before and after deselecting item1
JessThrysoee Mar 6, 2017
f780480
Cleanup component by unmounting wrapper in afterEach
JessThrysoee Mar 7, 2017
aa9dbdd
Add SelectField selectionRenderer test
JessThrysoee Mar 7, 2017
a3092fa
require react-tap-event-plugin for karma tests
JessThrysoee Mar 7, 2017
21e8514
Only persist event when necessary.
JessThrysoee Mar 7, 2017
985c477
Add a little documentation
JessThrysoee Mar 7, 2017
ba66177
Add ExampleSelectionRenderer
JessThrysoee Mar 8, 2017
f024b15
Remove multiple guard in handleTouchTapControl
JessThrysoee Mar 8, 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import React, {Component} from 'react';
import SelectField from 'material-ui/SelectField';
import MenuItem from 'material-ui/MenuItem';

const names = [
'Oliver Hansen',
'Van Henry',
'April Tucker',
'Ralph Hubbard',
'Omar Alexander',
'Carlos Abbott',
'Miriam Wagner',
'Bradley Wilkerson',
'Virginia Andrews',
'Kelly Snyder',
];

/**
* `SelectField` can handle multiple selections. It is enabled with the `multiple` property.
*/
export default class SelectFieldExampleMultiSelect extends Component {
state = {
values: [],
};

handleChange = (event, index, values) => this.setState({values});

menuItems(values) {
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid that function indirection, but I'm fine either way.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I'll add it inline.

return names.map((name) => (
<MenuItem
key={name}
insetChildren={true}
checked={values && values.includes(name)}
value={name}
primaryText={name}
/>
));
}

render() {
const {values} = this.state;
Copy link
Member

Choose a reason for hiding this comment

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

Some would consider destructuration overkill here, I'm fine either way.

Copy link
Author

Choose a reason for hiding this comment

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

OK

return (
<SelectField
multiple={true}
hintText="Select a name"
value={values}
onChange={this.handleChange}
>
{this.menuItems(values)}
</SelectField>
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React, {Component} from 'react';
import SelectField from 'material-ui/SelectField';
import MenuItem from 'material-ui/MenuItem';

const persons = [
{value: 0, name: 'Oliver Hansen'},
{value: 1, name: 'Van Henry'},
{value: 2, name: 'April Tucker'},
{value: 3, name: 'Ralph Hubbard'},
{value: 4, name: 'Omar Alexander'},
{value: 5, name: 'Carlos Abbott'},
{value: 6, name: 'Miriam Wagner'},
{value: 7, name: 'Bradley Wilkerson'},
{value: 8, name: 'Virginia Andrews'},
{value: 9, name: 'Kelly Snyder'},
];

/**
* The rendering of selected items can be customized by providing a `selectionRenderer`.
*/
export default class SelectFieldExampleSelectionRenderer extends Component {
state = {
values: [],
};

handleChange = (event, index, values) => this.setState({values});

selectionRenderer = (values) => {
switch (values.length) {
case 0:
return '';
case 1:
return persons[values[0]].name;
default:
return `${values.length} names selected`;
}
}

menuItems(persons) {
return persons.map((person) => (
<MenuItem
key={person.value}
insetChildren={true}
checked={this.state.values.includes(person.value)}
value={person.value}
primaryText={person.name}
/>
));
}

render() {
return (
<SelectField
multiple={true}
hintText="Select a name"
value={this.state.values}
onChange={this.handleChange}
selectionRenderer={this.selectionRenderer}
>
{this.menuItems(persons)}
</SelectField>
);
}
}
16 changes: 16 additions & 0 deletions docs/src/app/components/pages/components/SelectField/Page.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ import SelectFieldExampleError from './ExampleError';
import selectFieldExampleErrorCode from '!raw!./ExampleError';
import SelectFieldExampleNullable from './ExampleNullable';
import SelectFieldExampleNullableCode from '!raw!./ExampleNullable';
import SelectFieldExampleMultiSelect from './ExampleMultiSelect';
import selectFieldExampleMultiSelectCode from '!raw!./ExampleMultiSelect';
import SelectFieldExampleSelectionRenderer from './ExampleSelectionRenderer';
import selectFieldExampleSelectionRendererCode from '!raw!./ExampleSelectionRenderer';
import selectFieldCode from '!raw!material-ui/SelectField/SelectField';

const SelectFieldPage = () => (
Expand Down Expand Up @@ -60,6 +64,18 @@ const SelectFieldPage = () => (
>
<SelectFieldExampleError />
</CodeExample>
<CodeExample
title="Multiple selection example"
code={selectFieldExampleMultiSelectCode}
>
<SelectFieldExampleMultiSelect />
</CodeExample>
<CodeExample
title="Selection renderer example"
code={selectFieldExampleSelectionRendererCode}
>
<SelectFieldExampleSelectionRenderer />
</CodeExample>
<PropTypeDescription code={selectFieldCode} />
</div>
);
Expand Down
90 changes: 75 additions & 15 deletions src/DropDownMenu/DropDownMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,20 @@ class DropDownMenu extends Component {
* Overrides the styles of `Menu` when the `DropDownMenu` is displayed.
*/
menuStyle: PropTypes.object,
/**
* If true, `value` must be an array and the menu will support
* multiple selections.
*/
multiple: PropTypes.bool,
/**
* Callback function fired when a menu item is clicked, other than the one currently selected.
*
* @param {object} event TouchTap event targeting the menu item that was clicked.
* @param {number} key The index of the clicked menu item in the `children` collection.
* @param {any} payload The `value` prop of the clicked menu item.
* @param {any} value If `multiple` is true, the menu's `value`
* array with either the menu item's `value` added (if
* it wasn't already selected) or omitted (if it was already selected).
* Otherwise, the `value` of the menu item.
*/
onChange: PropTypes.func,
/**
Expand All @@ -158,6 +166,15 @@ class DropDownMenu extends Component {
* Override the inline-styles of selected menu items.
*/
selectedMenuItemStyle: PropTypes.object,
/**
* Callback function fired when a menu item is clicked, other than the one currently selected.
*
* @param {any} value If `multiple` is true, the menu's `value`
* array with either the menu item's `value` added (if
* it wasn't already selected) or omitted (if it was already selected).
* Otherwise, the `value` of the menu item.
*/
selectionRenderer: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that name 👍 . That might miss a unit test.

Copy link
Author

Choose a reason for hiding this comment

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

Agree, that needs to be tested.

/**
* Override the inline-styles of the root element.
*/
Expand All @@ -167,7 +184,9 @@ class DropDownMenu extends Component {
*/
underlineStyle: PropTypes.object,
/**
* The value that is currently selected.
* If `multiple` is true, an array of the `value`s of the selected
* menu items. Otherwise, the `value` of the selected menu item.
* If provided, the menu will be a controlled component.
*/
value: PropTypes.any,
};
Expand All @@ -179,6 +198,7 @@ class DropDownMenu extends Component {
iconButton: <DropDownArrow />,
openImmediately: false,
maxHeight: 500,
multiple: false,
};

static contextTypes = {
Expand Down Expand Up @@ -273,16 +293,28 @@ class DropDownMenu extends Component {
};

handleItemTouchTap = (event, child, index) => {
Copy link
Member

@oliviertassinari oliviertassinari Feb 21, 2017

Choose a reason for hiding this comment

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

I haven't realized that the Menu component already handles a multiple property.

With that in mind, why is onItemTouchTap triggering the onChange event. Should it be decoupled and handled aside? I mean in the handleOnChange function.

I feel like we have quite some logic duplication.

Copy link
Author

Choose a reason for hiding this comment

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

Yes the idea is to expose Menu's existing multiple functionality.

Menu has two callbacks onChange and onItemTouchTap. In the current master branch Menu's onItemTouchTab is propagated up through DropDownMenu's and SelectField's onChange callbacks. So to avoid breaking existing clients I thought that was best left unchanged.

Menu's onChange is already using the semantics that if multiple is true then onChange is called with arrays, so I did the same for SelectField and DropDownMenu.

The logic duplication is mostly in Menu. If breaking changes could be made then I think Menu's onItemTouchTab should be removed in favor of an updated onChange with a richer set of parameters.

Copy link
Member

Choose a reason for hiding this comment

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

The logic duplication is mostly in Menu. If breaking changes could be made then I think Menu's onItemTouchTab should be removed in favor of an updated onChange with a richer set of parameters.

I agree but I would definitely avoid introducing breaking changes on the master. We have the next branch to move forward. That current logic sounds like a good tradeoff for now.

event.persist();
this.setState({
open: false,
}, () => {
if (this.props.onChange) {
this.props.onChange(event, index, child.props.value);
if (this.props.multiple) {
if (!this.state.open) {
this.setState({open: true});
}
} else {
event.persist();
this.setState({
open: false,
}, () => {
if (this.props.onChange) {
this.props.onChange(event, index, child.props.value);
}

this.close(Events.isKeyboard(event));
});
this.close(Events.isKeyboard(event));
});
}
};

handleChange = (event, value) => {
if (this.props.multiple && this.props.onChange) {
this.props.onChange(event, undefined, value);
}
};

close = (isKeyboard) => {
Expand All @@ -307,6 +339,7 @@ class DropDownMenu extends Component {
animated,
animation,
autoWidth,
multiple,
children,
className,
disabled,
Expand All @@ -315,6 +348,7 @@ class DropDownMenu extends Component {
listStyle,
maxHeight,
menuStyle: menuStyleProp,
selectionRenderer,
onClose, // eslint-disable-line no-unused-vars
openImmediately, // eslint-disable-line no-unused-vars
menuItemStyle,
Expand All @@ -334,12 +368,36 @@ class DropDownMenu extends Component {
const styles = getStyles(this.props, this.context);

let displayValue = '';
React.Children.forEach(children, (child) => {
if (child && value === child.props.value) {
// This will need to be improved (in case primaryText is a node)
displayValue = child.props.label || child.props.primaryText;
if (!multiple) {
React.Children.forEach(children, (child) => {
if (child && value === child.props.value) {
if (selectionRenderer) {
displayValue = selectionRenderer(value);
} else {
// This will need to be improved (in case primaryText is a node)
displayValue = child.props.label || child.props.primaryText;
}
}
});
} else {
const values = [];
React.Children.forEach(children, (child) => {
if (child && value && value.includes(child.props.value)) {
if (selectionRenderer) {
values.push(child.props.value);
} else {
values.push(child.props.label || child.props.primaryText);
}
}
});

displayValue = [];
if (selectionRenderer) {
displayValue = selectionRenderer(values);
} else {
displayValue = values.join(', ');
}
});
}

let menuStyle;
if (anchorEl && !autoWidth) {
Expand Down Expand Up @@ -385,13 +443,15 @@ class DropDownMenu extends Component {
onRequestClose={this.handleRequestCloseMenu}
>
<Menu
multiple={multiple}
maxHeight={maxHeight}
desktop={true}
value={value}
onEscKeyDown={this.handleEscKeyDownMenu}
style={menuStyle}
listStyle={listStyle}
onItemTouchTap={this.handleItemTouchTap}
onChange={this.handleChange}
menuItemStyle={menuItemStyle}
selectedMenuItemStyle={selectedMenuItemStyle}
>
Expand Down
54 changes: 53 additions & 1 deletion src/DropDownMenu/DropDownMenu.spec.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-env mocha */
import React, {PropTypes} from 'react';
import React, {PropTypes, Component} from 'react';
import {shallow, mount} from 'enzyme';
import {assert} from 'chai';
import keycode from 'keycode';
Expand All @@ -9,6 +9,7 @@ import getMuiTheme from '../styles/getMuiTheme';
import MenuItem from '../MenuItem';
import Menu from '../Menu/Menu';
import IconButton from '../IconButton';
import TestUtils from 'react-addons-test-utils';

describe('<DropDownMenu />', () => {
const muiTheme = getMuiTheme();
Expand Down Expand Up @@ -185,4 +186,55 @@ describe('<DropDownMenu />', () => {
assert.strictEqual(wrapper.state().open, true, 'it should open the menu');
});
});

describe('MultiSelect', () => {
let wrapper;

it('should multi select 2 items after selecting 3 and deselecting 1', () => {
class MyComponent1 extends Component {
state = {
value: null,
}

handleChange = (event, key, value) => {
this.setState({value});
}

render() {
return (
<DropDownMenu
multiple={true}
value={this.state.value}
onChange={this.handleChange}
>
<MenuItem className="item1" value="item1" primaryText="item 1" />
<MenuItem className="item2" value="item2" primaryText="item 2" />
<MenuItem className="item3" value="item3" primaryText="item 3" />
</DropDownMenu>
);
}
}
wrapper = mountWithContext(<MyComponent1 />);
wrapper.find('IconButton').simulate('touchTap'); // open

const item1 = document.getElementsByClassName('item1')[0];
assert.ok(item1);
const item2 = document.getElementsByClassName('item2')[0];
assert.ok(item2);
const item3 = document.getElementsByClassName('item3')[0];
assert.ok(item3);

TestUtils.Simulate.touchTap(item1);
TestUtils.Simulate.touchTap(item2);
TestUtils.Simulate.touchTap(item3);
assert.deepEqual(wrapper.state().value, ['item1', 'item2', 'item3']);

TestUtils.Simulate.touchTap(item1); // deselect
assert.deepEqual(wrapper.state().value, ['item2', 'item3']);
});

afterEach(function() {
if (wrapper) wrapper.unmount();
});
});
});
Loading