myforce / angularjs-dropdown-multiselect

AngularJS Dropdown Multiselect
http://myforce.github.io/angularjs-dropdown-multiselect/
MIT License
34 stars 28 forks source link

Add the ability for group selection #10

Closed skiminock closed 8 years ago

skiminock commented 8 years ago

Hello! I added the ability to select groups! To do this, set the optional parameter "groups" (usual string array). This was done for such case.

js:

$scope.data = [
{id: 1, label: "David", sex: 'Man'}, 
{id: 2, label: "Anna", sex: 'Woman'}, 
{id: 3, label: "Alex ", sex: 'Man'}];
$scope.groups = [ 'Man', 'Woman'];

html:

<div ng-dropdown-multiselect="" 
        ...
        group-by="sex" 
        groups = "groups">
</div>

Now you can select all men or all women at one time.

P.S. Your work is great! I'm using your fork in my project! Thank you! P.P.S Will be waiting your answer!

pkempenaers commented 8 years ago

Hi @morow93 ,

I feel like this could be a very useful feature, I however think that passing the groups shouldn't be needed. There is already the possibility to pass the property by which you want to group.

What i suggest is that you remove the groups attribute, add a setting 'allowSelectGroups' and add ng-click="selectCurrentGroup..." to the group headers. (so no separate items to select the groups) In the selectCurrentGroup you check if the allowSelectGroups is enabled and then select the group.

I also don't think that we need extra styling for the selected group (as all the items of that group will be selected) we can however easily do this.

I'm happy to hear from you if you don't agree with this, if you want I can also help you implementing this and then we merge these changes into master.

skiminock commented 8 years ago

Good day, @pkempenaers ! I understood you and I committed new changes! But I left "groups" attribute (but changed its name). In my case I needed to select by not all existing groups. I have people who did not specify their gender ("sex" for this people is "N\A" and I don't want to see option "Select all N\A" I just wanna see options "Men" and "Women"). And now "selectedByGroups" is array of object. Element of array should has two properties - value (using for selecting\deselecting) and title (using for displaying). Also I fixed some bugs with divider which appeared in my previous commit. I await your verdict) Also I'm ready for discussion)

skiminock commented 8 years ago

@pkempenaers what about my changes?)

pkempenaers commented 8 years ago

Sorry, for the few days of no response, I've been busy at home and workload at work is rather high the last few days. I'm looking to rework some things strongly based on your PR and then merge it in. Should be done before the end of this week.

skiminock commented 8 years ago

OK!)

skiminock commented 8 years ago

@pkempenaers Also I added the ability to disable multiselect. And one more thing - I noticed that you using "id" attributes in template. I think the better way is using "class" instead)

pkempenaers commented 8 years ago

There are indeed a few things wrong with this directive from a best practices stand point :), I haven't found the time to change some things and for example implement some unit tests. These are however on my "whishlist"

skiminock commented 8 years ago

I watched directive code and sometimes I want to cry) I know, it is fork. And I have no pretensions to you)

pkempenaers commented 8 years ago

First of all thanks for your patience on this.

I've merged in parts of this pull request (your first 2 commits) after which i've changed some things. The last commit I've cherry picked. The problem with this is that your pull request isn't fully merged in according to github and your last commit doesn't count as a contribution (the first two however do).

This pull request is in Version 1.9.0 and higher. Some points to note: For the select by to work you need:

The disabled works as you've implemented it.

Thank you for your contributions to this component.

skiminock commented 8 years ago

Thank you for your time)))