Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

Support for multi-selection #138

Merged
merged 18 commits into from
Sep 8, 2014
Merged

Support for multi-selection #138

merged 18 commits into from
Sep 8, 2014

Conversation

dimirc
Copy link
Contributor

@dimirc dimirc commented Aug 4, 2014

WIP:

Based on work done at #78 by @komakino. I manually rebased and did some minor changes.

TODO:

  • Add selectize template (maybe dropping support for this theme? Dropping support for 'selectize' theme? #175)
  • Add bootstrap template
  • Change transinject for uisTranscludeAppend from fix(transclude): compile transclude elements with correct scope #151
  • Improve margins between buttons at bootstrap theme
  • Write tests
  • Remove items from drop down that are already selected
  • Check formatter/parser (single property binding) to work correctly on "multiple" mode
  • Check choices with groupby to work correctly on "multiple" mode
  • Fix: multiple choices highlighted at same time
  • Fix: dropdown doesn't appear when pressing key down (just the word "true")
  • Fix: when pressing a key on the input, the whole selected array is erased
  • Prevent already selected items to show when searching
  • Refactor/Clean code
  • Add demo
  • Avoid $setViewValue third parameter as explained on this comment
  • Fix: refresh function, every new search removes items selected earlier.

Demo plunker (I'll be updating it with each new commit)

@dimirc
Copy link
Contributor Author

dimirc commented Aug 4, 2014

@komakino can you give more info on what your meant with these 2 other tasks that you had on the PR:

  • A user provided compare function to determine if an initially selected item matches an item in ctrl.items. Currently uses angular.equals
  • Remove Tab key events; they were illogical and prevented ui-select from functioning correctly in a form

@PowerKiKi
Copy link

On the plunker you submitted, there is a visual bug where elements are highlighted when they should not be. Do the following:

  1. click in input (to give focus)
  2. type "w"
  3. click on the item "COLOR = Yellow" (to select it)
  4. click in input (to re-give focus)
  5. the item list will be shown with several highlighted items, but I believe none of them should be highlited at this point:

image

@dimirc
Copy link
Contributor Author

dimirc commented Aug 4, 2014

@PowerKiKi yes, this first commit was basically just the rebase of the code at the other PR. I see several some bug that I'll start fixing with next commits.

@dimirc
Copy link
Contributor Author

dimirc commented Aug 4, 2014

@PowerKiKi I just push new commit that solves this problem (plunker updated)

@PowerKiKi
Copy link

That was fast, thanks ! looking forward to seeing this merged :-)

@jgoux
Copy link

jgoux commented Aug 4, 2014

This is huge, thanks a lot for your hard work!

@dimirc
Copy link
Contributor Author

dimirc commented Aug 5, 2014

I just pushed a tricky commit to be able to have the elements of the model (array), bind only to single property instead of whole object like #107.

To make it clearer, plunker demo is showing different examples now.

@brafkind
Copy link

brafkind commented Aug 5, 2014

Merge please!

@dimirc
Copy link
Contributor Author

dimirc commented Aug 11, 2014

Plunker for Bootstrap theme

@just-boris
Copy link
Contributor

Hello all.
I don't think that this feature is a good idea. I already use another component for multiple selects  — ngTagsInput and it looks good for me.
My opinion is that multiple select claims another API (you need to return array as model value) and have different use cases than select, so the source code becomes very difficult to maintain and more hostile for new features.
So I appreciate two similar libraries and both of that can do own work well.

@brafkind
Copy link

just-boris, ngTagsInput is great, but it only shows options once you start typing. However, it doesn't allow the user to click into the field and immediately see a drop-down of choices, like we'd get with this enhancement, so I believe it's still necessary.

Once this is merged, how is one expected to configure the component to get multiple selection? Is there an example?

@dimirc
Copy link
Contributor Author

dimirc commented Aug 11, 2014

@just-boris keep in mind that even the native <select> element also handles multiple so I think this feature is something we should also address, I feel that it's logical to have one library for single/multiple selections. We added it to our roadmap long time ago, keeping in mind that we want to be able to solve the common "selectable" needs that a regular app could have.

I just did a quick check to ngTagsInput and I see some differences, for example:

  • We'll be able to bind to a single property of the objects instead of the whole object, not sure if the other directive lets you do that.
  • In their examples I'm checking that I can't navigate through the tags with the arrow left/right to select a particular item and possible to erase it, we can do it.
  • I see that they have a display-property attribute to specify a property to use when displaying tags but and we have the <match> element that give us much more freedom to display

@brafkind you only need to add multipleattribute and this will work and return an array to the ng-model instead of a single value.

  <ui-select multiple ng-model="multipleDemo.selectedPeopleSimple">
    <ui-select-match placeholder="Select person...">{{$item.name}} &lt;{{$item.email}}&gt;</ui-select-match>
    <ui-select-choices repeat="person.email as person in people | propsFilter: {name: $select.search, age: $select.search}">
      <div ng-bind-html="person.name | highlight: $select.search"></div>
      <small>
        email: {{person.email}}
        age: <span ng-bind-html="''+person.age | highlight: $select.search"></span>
      </small>
    </ui-select-choices>
  </ui-select>

@dimirc dimirc added this to the 0.6.x milestone Aug 11, 2014
result;
if ($select.multiple){
var resultMultiple = [];
for (var j = inputValue.length - 1; j >= 0; j--) {
Copy link
Contributor

Choose a reason for hiding this comment

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

input value comes from user, we should check that it is array before use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@just-boris solved on this commit 22f67aa

@just-boris
Copy link
Contributor

@dimirc you reasons seem convincing.
I've added multiple select with groups in suggest into the plunker. I don't see any conflicts there

@keepitsimple
Copy link

Thank you! It's a very important feature.

  1. Remove already selected items while searching must be an optional parameter, e.g. I want to select several addresses.

  2. selectedItems arrya can be empty. The "!selectedItems" check is not enough.

    //Remove already selected items 
    $scope.$watchCollection('$select.selected', function(selectedItems){
      if (!selectedItems || selectedItems.length == 0) return; 

@keepitsimple
Copy link

Could you please point me at the place in the code where the refresh function in the multi ui-select removes items in the selected list?

@dimirc
Copy link
Contributor Author

dimirc commented Aug 20, 2014

  1. Remove already selected items while searching must be an optional parameter, e.g. I want to select several addresses.

@keepitsimple What you mean that you want to select several addresses? The same address many times? Can you give a better example of the use case were this could be optional?

@dimirc
Copy link
Contributor Author

dimirc commented Sep 4, 2014

If anyone wants to help, writing test for this PR will help a lot


ctrl.selected = item;
if(ctrl.multiple){
if(!_itemInSelected(item)){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be necessary since we are already filtering selected

@dimirc
Copy link
Contributor Author

dimirc commented Sep 8, 2014

@keepitsimple I just pushed a commit that should correct the problem you had

@dimirc
Copy link
Contributor Author

dimirc commented Sep 8, 2014

I created #176 to check how to fix the use of 3rd parameter on $setViewValue as explained in previous comment. For now this option will let this PR to work correctly with versions up to v1.3.0-rc.0

dimirc added a commit that referenced this pull request Sep 8, 2014
@dimirc dimirc merged commit 5787c04 into master Sep 8, 2014
@dimirc dimirc deleted the feat-multiple branch September 8, 2014 23:46
@dimirc dimirc removed the in progress label Sep 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants