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

[AutoComplete] Add multiselect & selectedOptions features + improvements #5018

Closed
wants to merge 12 commits into from
Closed

Conversation

Sharlaan
Copy link

@Sharlaan Sharlaan commented Aug 18, 2016

AutoComplete with multiple selections and pre-selections

  • PR has tests.
  • PR has docs demo,
  • PR is linted (babel-eslint, StandardJS), dev'd with Webstorm 2016.2.2.
  • PR title begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s)

Demo

CodeExample page showing all 3 usecases:
a. single value Autocomplete
b. multiselect autocomplete (with checkboxes)
c. multiselect autocomplete (without checkboxes and with MenuItem nodes)
An external container below the 3 autocompletes shows the selected options with Chips

usage

New properties

  • multiple : Default: false. If set to true, the first argument of onNewRequest's signature will become an array. Its content will be the same as of dataSource, ie array of Strings if dataSource is an Array[String], array of objects or array of nodes.
  • withCheckboxes : Default: true. If multiple is true, the dropdown menu will use checkboxes. If multiple===true && withCheckboxes===false, selecting an option hides it from the popover.
  • selectedOptions : Default: []. If multiple is true, the component's internal state will use this property to set preselectedOptions during componentWillMount phase. This property can also be used to notify the component of externally selected values (example: deselecting a Chip'ed value will notify the component to uncheck corresponding checkbox).
    Note: in all cases, the textfield responsibility is only for searching terms, displaying results is externalized to another component, via onNewRequest(results, index).
  • dataSourceConfig now has a new 'node' property, more explicit when developper wants to use a node instead of a string.

Improvements

  • removed timeout hack between popover's focus and searchTextField's onBlur. UIevents (W3C) defines Blur phase triggering always before Focus, so i added in onBlur handler a check if the focused node is one of the popover's children, or anything else (clickaway), using relatedTarget.
  • rewrote the code with ES7.
  • refactored the requestsList rendering.

Known issues

  • Untested under mobile environmentsand Edge (tested on Chrome52 and Firefox48)

Related issues

Alternative implementation: #4252
closes #4952
closes #1956

Please report me any bugs or suggestions !

if needed, i have a CodeExample showing all 3 usecases:
a. single value Autocomplete
b. multiselect autocomplete (with checkboxes)
c. multiselect autocomplete (without checkboxes and with MenuItem nodes)
An external container below the 3 autocompletes shows the selected options with Chips

known issues:
- When deselecting an option from a Chip, CodeExample doesnot notify the corresponding AutoComplete
- UseCase c: Using WithCheckboxes with node type items makes check marks horribly unlined with the item's contents
- Untested under mobile environments
@muibot muibot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 18, 2016
Demo showing 3 usecases:
- single value autocomplete
- AutoComplete multiple selections with checkboxes and preselected values
- AutoComplete multiple selections without checkboxes with node type items
changed back state.preSelectedOptions into props.selectedOptions for easier notifications from parent to child.
@nomadtechie
Copy link

@Sharlaan are you still planning on getting this merged in? Anything I can do to help with the merge conflicts? Would love to see this feature get into the next release?

@Sharlaan
Copy link
Author

Hi Amala, it would be nice if the M-UI team accepts this PR for merge but i don't know how to resolve those conflicts which arenot even pointed out ...

Feel free to teach me, i never did any merge.

I suspect these conflicts coming from the fact my build arrived "late" compared to the master's version, but i might be wrong and don't know the solution.

@Sharlaan
Copy link
Author

Just found an excellent alternative:
https://github.com/TeamWertarbyte/material-ui-chip-input

@nomadtechie
Copy link

@Sharlaan sorry I just saw this message--I can resolve the conflicts for you. How complete is your PR with test coverage, etc..?

@nomadtechie
Copy link

@leMaik love your library (material-ui-chip-input) would you be open to submitting a PR to this project to add it?

@Sharlaan
Copy link
Author

@amalahussein
There are no tests (except manual ones) nor coverage.
For now i'm trying to reduce all 12 commits into one single, this should simplify next steps (rebase or cherry-pick) ...

@leMaik
Copy link
Member

leMaik commented Sep 22, 2016

@amalahussein I'd love to! :) But there are still some features missing that I'd like to add before submitting a PR (see the issues). Also, there are no tests (except for storybook stories) at the moment.

Regarding the linter: Do you really use StandardJS? I saw loads of semicolons in the code... 😮

Edit: This chip input would greatly benefit from this PR, because an AutoComplete field that could stay open is exactly what it would need...

@Sharlaan
Copy link
Author

"Regarding the linter: Do you really use StandardJS? I saw loads of semicolons in the code..."
yes i do, that's 1st thing i do when refactoring someone else code: removing all semicolons.

@leMaik
Copy link
Member

leMaik commented Sep 22, 2016

@Sharlaan I just wondered because material-ui doesn't use StandardJS. They have semicolons and dangling commas everywhere. I don't think that this will get merged unless it follows their linter settings. 😉

@Sharlaan
Copy link
Author

Dang... good point :(

@cvanleeuwen
Copy link

@Sharlaan does this mean it's abandoned?

@leMaik
Copy link
Member

leMaik commented Oct 14, 2016

@Sharlaan Did you just close this because the code isn't linted? 😮

@Sharlaan
Copy link
Author

Sharlaan commented Oct 15, 2016

It's not abandoned but i'm struggling setting up, keeping the fork up-to-date, and integrating the project with the MUI environment... especially the next branch (no point to do it for the current version 0.16 ?)

I also decided to rewrite from scratch to propose an all-in-one SelectField :

  • SelectField+AutoComplete+DropDown
  • Composable with any children (requires a value prop, and a label prop if autoComplete is enabled)
  • Exposes a selectionsRenderer, and a filterFunction
  • Allows to change CheckIconFalse CheckIconTrue (when multiple enabled)

Current webpackbin here.
or my repo.

EDIT: i just noticed this nice idea, i'll try to implement it as well.

@Sharlaan
Copy link
Author

Just another nice react-autosuggest, though not multiSelectable, it supports optgroups and other niceties.

@lpopescu
Copy link

lpopescu commented Aug 8, 2017

Any update on this ?

EDIT: i just noticed this nice idea, i'll try to implement it as well.

I did implemented the same thing but I had some issues and it didn't worked well in the end. I want to know if you were more successful 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants