[SelectField] [DropDownMenu] Support multi select #6165

JessThrysoee · 2017-02-16T14:01:37Z

Multi select in SelectField is requested in #1956, but progress doesn't seem to have been made (I'm new to material-ui so I might just be uninformed :-).

To get the ball rolling, this PR simply makes Menus multi select support available in SelectField and DropDownMenu. A demo of what it looks like can be seen here: http://thrysoee.dk/material-ui/

I haven't looked into updating docs/tests/... yet. I thought I'd hear what you guys think before going any further.

oliviertassinari · 2017-02-19T11:09:20Z

@JessThrysoee I like the direction this PR is taking. The multiple property name sounds better to the multi one used by react-select.

I haven't looked into updating docs/tests/... yet. I thought I'd hear what you guys think before going any further.

Increasing the test coverage is one of our priority with the next branch. So doing the same with the master would be great.

@Sharlaan I would love to have your feedback on this PR. Would that help with material-ui-superselectfield?

Sharlaan · 2017-02-19T12:53:52Z

Nice to see another alternative!

At first glance, it doesnot help me much since i'm already quite ahead with features.

Regarding multiple i'm currently adding another prop to complement it : checkPosition (default: '') PropTypes.oneOf(['', 'left', 'right'])
Some people may not want the check mark, or want it as right icon.

Regarding datasource, what if dev wants to display a label different than its associated value ? (ie datasource as ArrayOf[Object])

Regarding the display of selections, this uses the classic one with joined values string + ellipsis.
What if user want to see full list ?
Also how to display those selections in a more fashion way ?

Finally, what's the difference between DropDown and SelectField components ? why not merge them into one same component ?

Hope it helps.

At the moment i'm struggling with ListItem's focusState to implement the keyboard gestures, and hoverColor/Ripple states.
Your thoughts ?

JessThrysoee · 2017-02-19T18:04:06Z

Thanks for the feedback!

I agree that we shouldn't assume that the user want a check mark toggled, so this PR doesn't affect the MenuItems at all :-) It only makes Menu's multiple prop available and returns an array of values in onChange when multiple is true. It is up to the user what should be updated on the MenuItems.

Regarding datasource and display of selections: Yes, the user should absolutely be able to render the selection. I have just pushed a small change that adds an onChangeRenderer prop, so the user has full control over the rendering. I have updated the demo to show onChangeRenderer returning a Component instead of a string - http://thrysoee.dk/material-ui/ .

(This PR tries to be a small but useful increment of functionality)

BTW: The lint is currently failing on files unrelated to this PR.

Sharlaan · 2017-02-19T18:14:43Z

in my project, i actually called your onChangeRenderer as selectionsRenderer, up to you ;)

My comment about the check mark is not inherent to your PR, but to the MenuItem itself, which assumes the user want a check mark :s
This would need another PR in my opinion.

got same linting issues, but there are 2 PRs fixing them in the PRs queue XD

JessThrysoee · 2017-02-20T17:29:04Z

Yes. selectionsRenderer is a better name. This is not an event handler so onChangeRenderer is misleading. I think maybe for this use the singular selectionRenderer is more in line with the 'value' prop.

oliviertassinari

That's a good start 👍 . Thanks for working on it. Aside from my comment, we gonna need some tests and a documentation example to get it merged.

oliviertassinari · 2017-02-21T20:55:57Z

src/DropDownMenu/DropDownMenu.js

@@ -274,15 +294,28 @@ class DropDownMenu extends Component {

  handleItemTouchTap = (event, child, index) => {


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.

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.

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.

oliviertassinari · 2017-02-21T20:56:18Z

src/DropDownMenu/DropDownMenu.js

            maxHeight={maxHeight}
            desktop={true}
            value={value}
            onEscKeyDown={this.handleEscKeyDownMenu}
            style={menuStyle}
            listStyle={listStyle}
            onItemTouchTap={this.handleItemTouchTap}
+            onChange={this.handleOnChange}


We have the following naming convention:

onChange={this.handleChange}

JessThrysoee · 2017-02-22T23:56:02Z

I'm having trouble writing tests for DropDownMenu. I mountWithContext and simulate('touchTap') to open the Popover, but enzyme debug tells me that there are no MenuItems to select.

<DropDownMenu >
  <div >
    <ClearFix >
    </ClearFix>
    <Popover  open={true} >
      <div >
        <EventListener target="window" />
        <RenderToLayer open={true} />
      </div>
    </Popover>
  </div>
</DropDownMenu>

I suspect it has something to do with RenderToLayer waiting until componentDidMount to call its renderLayer method? Any hints to how I can test selecting a MenuItem would be greatly appreciated!

@oliviertassinari Would it be sufficient to write multi select tests for Menu given that most of the functionality is handled there?

oliviertassinari

Any hints to how I can test selecting a MenuItem would be greatly appreciated!

I think that it's linked to the Popover component rendering the component outside of his DOM parent.
The classname workaround is clever. #6290 is also looking for a better solution.

oliviertassinari · 2017-03-06T22:18:45Z

docs/src/app/components/pages/components/SelectField/ExampleMultiSelect.js

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


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

OK. I'll add it inline.

oliviertassinari · 2017-03-06T22:19:08Z

docs/src/app/components/pages/components/SelectField/ExampleMultiSelect.js

+  }
+
+  render() {
+    const {values} = this.state;


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

oliviertassinari · 2017-03-06T22:19:28Z

docs/src/app/components/pages/components/SelectField/Page.js

@@ -60,6 +62,14 @@ const SelectFieldPage = () => (
    >
      <SelectFieldExampleError />
    </CodeExample>
+


We avoid blank line in the render method of the components.

oliviertassinari · 2017-03-06T22:21:33Z

src/DropDownMenu/DropDownMenu.js

@@ -241,7 +261,7 @@ class DropDownMenu extends Component {

  handleTouchTapControl = (event) => {
    event.preventDefault();
-    if (!this.props.disabled) {
+    if (!this.props.disabled && !(this.props.multiple && this.state.open)) {


What is this logic for?

When multiple is true, the dropdown shouldn't be closed on touch. We wan't to leave it open for further selections.

From my own tests, that callback isn't called when the dropdown is touched. I think that it's a dead logic.

You are right. The ClearFix component is covered by the Popover so no touch events are possible.

oliviertassinari · 2017-03-06T22:23:31Z

src/DropDownMenu/DropDownMenu.js

@@ -274,15 +294,28 @@ class DropDownMenu extends Component {

  handleItemTouchTap = (event, child, index) => {
    event.persist();


I think that we can move that line in the else clause.

I have moved it.

oliviertassinari · 2017-03-06T22:24:38Z

src/DropDownMenu/DropDownMenu.js

@@ -274,15 +294,28 @@ class DropDownMenu extends Component {

  handleItemTouchTap = (event, child, index) => {


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.

oliviertassinari · 2017-03-06T22:26:33Z

src/DropDownMenu/DropDownMenu.spec.js

+      TestUtils.Simulate.touchTap(item1);
+      TestUtils.Simulate.touchTap(item2);
+      TestUtils.Simulate.touchTap(item3);
+      TestUtils.Simulate.touchTap(item1);  // deselect


What about asserting the state before the deselect?

oliviertassinari · 2017-03-06T22:28:10Z

src/TextField/TextField.js

@@ -105,7 +105,7 @@ const getStyles = (props, context, state) => {
 * @returns True if the string provided is valid, false otherwise.
 */
 function isValid(value) {
-  return value !== '' && value !== undefined && value !== null;
+  return value !== '' && value !== undefined && value !== null && !(Array.isArray(value) && value.length === 0);


Sounds like an abstraction leak. Can't let the TextField untouched?

Yes. I agree that it look a little odd, but I added it so SelectField's value can be initialized to an empty array (as well as null) which feels more natural when multiple is true. Without this test TextField always thinks it has a value and hintText and floatingLabelText doesn't behave as expected.

I 100% agree with the motivation. My point is about moving the piece of logic into the DropDownMenu component.

Good point. I'll move it into DropDownMenu.

Hmm. TextField is reaching down into the child DropDownMenu to read its value, so I don't see a good way to avoid the empty array check in TextField.

@JessThrysoee Oh, I remember that. Well, I'm find keeping that hack as we are rewriting the lib on the next branch.

oliviertassinari · 2017-03-06T22:38:49Z

src/DropDownMenu/DropDownMenu.js

+     * it wasn't already selected) or omitted (if it was already selected).
+     * Otherwise, the `value` of the menu item.
+     */
+    selectionRenderer: PropTypes.func,


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

Agree, that needs to be tested.

oliviertassinari · 2017-03-06T22:40:54Z

@JessThrysoee That's a great PR, good job so far. Sorry for not reviewing it earlier.
We are close from getting it merged 🎉 .

JessThrysoee · 2017-03-06T23:46:04Z

I'm still having a problem with the SelectField.spec.js test. It runs successfully with npm run test:unit -- -c SelectField but fails with ./scripts/run-travis-tests.sh

It looks like TestUtils.Simulate.touchTap(item1); isn't working during the travis test run for SelectField.spec.js, but on the other hand it works fine in DropDownMenu.spec.js

JessThrysoee · 2017-03-08T20:46:07Z

Added an example of using selectionRenderer to the docs.

I think the PR is in good shape now, so I'll wait for review (or merge :) before doing anything more.

oliviertassinari

We are really close 🎉 .

oliviertassinari · 2017-03-08T21:23:45Z

src/DropDownMenu/DropDownMenu.js

@@ -241,7 +261,7 @@ class DropDownMenu extends Component {

  handleTouchTapControl = (event) => {
    event.preventDefault();
-    if (!this.props.disabled) {
+    if (!this.props.disabled && !(this.props.multiple && this.state.open)) {


From my own tests, that callback isn't called when the dropdown is touched. I think that it's a dead logic.

oliviertassinari · 2017-03-08T21:27:48Z

src/TextField/TextField.js

@@ -105,7 +105,7 @@ const getStyles = (props, context, state) => {
 * @returns True if the string provided is valid, false otherwise.
 */
 function isValid(value) {
-  return value !== '' && value !== undefined && value !== null;
+  return value !== '' && value !== undefined && value !== null && !(Array.isArray(value) && value.length === 0);


@JessThrysoee Oh, I remember that. Well, I'm find keeping that hack as we are rewriting the lib on the next branch.

The ClearFix component is covered by the Popover so it is not possible to interact with it.

oliviertassinari · 2017-03-11T14:37:23Z

@JessThrysoee Thanks a lot for your effort!

mbrookes · 2017-03-11T16:39:21Z

@JessThrysoee Thanks for closing the all-time most requested feature! 🎉

https://github.com/callemall/material-ui/issues?q=is%3Aissue+sort%3Areactions-%2B1-desc

JessThrysoee · 2017-03-12T07:52:39Z

@oliviertassinari @mbrookes Glad to have contributed. Thank you guys for creating and maintaining such a high quality project!

patrickml · 2017-03-15T21:11:23Z

This is really nice! What would it take to add an autocomplete search to something like this? I'd assume it'd just be a simple filter?

Sharlaan · 2017-03-15T21:13:21Z

I'm interested too with @patrickml question, this is the reason i made my SuperSelectField.

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 Feb 16, 2017

JessThrysoee changed the title ~~[SelectField] Add multi select to SelectField and DropDownMenu~~ [SelectField] [DropDownMenu] Support multi select Feb 18, 2017

oliviertassinari added component: select This is the name of the generic UI component, not the React module! new feature New feature or request and removed PR: Needs Review labels Feb 19, 2017

muibot added the PR: Needs Review label Feb 19, 2017

oliviertassinari requested changes Feb 21, 2017

View reviewed changes

oliviertassinari removed the PR: Needs Review label Feb 21, 2017

muibot added the PR: Needs Review label Feb 21, 2017

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 Feb 22, 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 PR: Needs Review and removed PR: Needs Review labels Feb 24, 2017

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 5, 2017

oliviertassinari reviewed Mar 6, 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 6, 2017

muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 6, 2017

JessThrysoee added 14 commits March 8, 2017 19:34

Handle initial null menuValue

e18f403

Add multi select test to Menu

af05629

Add multi select test to DropDownMenu

ada7f96

Add multi select test to SelectField

57e574b

Always pass value when a selectionRenderer is provided.

5df3184

Remove blank lines from render

564125f

Avoid function indirection and destructuration overkill

697deb4

Assert state before and after deselecting item1

b738e6e

Cleanup component by unmounting wrapper in afterEach

f780480

Add SelectField selectionRenderer test

aa9dbdd

require react-tap-event-plugin for karma tests

a3092fa

Only persist event when necessary.

21e8514

Add a little documentation

985c477

Add ExampleSelectionRenderer

ba66177

oliviertassinari reviewed Mar 8, 2017

View reviewed changes

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

Remove multiple guard in handleTouchTapControl

f024b15

The ClearFix component is covered by the Popover so it is not possible to interact with it.

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

oliviertassinari approved these changes Mar 8, 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 8, 2017

oliviertassinari merged commit 20325bc into mui:master Mar 11, 2017

JessThrysoee deleted the multiselectfield branch March 12, 2017 07:52

		@@ -274,15 +294,28 @@ class DropDownMenu extends Component {

		handleItemTouchTap = (event, child, index) => {


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

		menuItems(values) {

		@@ -274,15 +294,28 @@ class DropDownMenu extends Component {

		handleItemTouchTap = (event, child, index) => {
		event.persist();

[SelectField] [DropDownMenu] Support multi select #6165

[SelectField] [DropDownMenu] Support multi select #6165

Conversation

JessThrysoee commented Feb 16, 2017 • edited Loading

oliviertassinari commented Feb 19, 2017 • edited Loading

Sharlaan commented Feb 19, 2017 • edited Loading

JessThrysoee commented Feb 19, 2017

Sharlaan commented Feb 19, 2017

JessThrysoee commented Feb 20, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

JessThrysoee commented Feb 22, 2017 • edited by oliviertassinari Loading

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

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

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

Choose a reason for hiding this comment

oliviertassinari commented Mar 6, 2017

JessThrysoee commented Mar 6, 2017

JessThrysoee commented Mar 8, 2017

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 commented Mar 11, 2017

mbrookes commented Mar 11, 2017

JessThrysoee commented Mar 12, 2017

patrickml commented Mar 15, 2017

Sharlaan commented Mar 15, 2017

JessThrysoee commented Feb 16, 2017 •

edited

Loading

oliviertassinari commented Feb 19, 2017 •

edited

Loading

Sharlaan commented Feb 19, 2017 •

edited

Loading

oliviertassinari Feb 21, 2017 •

edited

Loading

oliviertassinari Feb 21, 2017 •

edited

Loading

JessThrysoee commented Feb 22, 2017 •

edited by oliviertassinari

Loading

oliviertassinari left a comment •

edited

Loading