isteven / angular-multi-select

A multi select dropdown directive for AngularJS. Allows you to use HTML tags and CSS in the data. Requires only AngularJS and nothing else.
isteven.github.io/angular-multi-select
MIT License
1.08k stars 518 forks source link

Binding/scope issue with ng-repeat #435

Closed nhobi closed 8 years ago

nhobi commented 8 years ago

Check out the Plunker: https://plnkr.co/edit/HsJcUO7NDSduqEVwahLH?p=preview

When I set the output model to a property on the ng-repeated item, it changes every item's property when I change the value instead of changing the property on just that item.

isteven commented 8 years ago

@gr33k01 ,

I'm sorry I don't really understand the question. What are you trying to achieve / what is the expected result of your code?

nhobi commented 8 years ago

Hey there,

I cleaned up the Plunker a bit: https://plnkr.co/edit/HsJcUO7NDSduqEVwahLH?p=preview

Basically, I'm using ng-repeat and setting the output model of your directive to dept.departmentHead for each of the repeated items.

I would expect that this control would work similarly to say, a <select> element, where if I changed the value on one one of the <select> elements, it would only update the dept.departmentHead on that one ng-repeat'd item.

However, as you can see in the Plunker, when I change the value on your control, it updates the property on all the ng-repeat items.

@isteven

isteven commented 8 years ago

That is because you're using the same output-model object name for all directive.

In javascript, the "=" operation means simply copying object reference. And since it's just a copy-by-reference, if you change one, the others will also change. A good explanation can be found at http://stackoverflow.com/questions/13937578/replace-a-one-javascript-object-with-another-object or http://stackoverflow.com/questions/15214378/why-does-updating-properties-in-one-object-change-another-object

To solve it, simply use different object name for the output-model.

veselj43 commented 8 years ago

Hi, i have the same problem, but i am not sure how would you solve it. The thing is how would you use different object name inside of a cycle (when variable isn't enough)? I'd say it basically means don't use a cycle.

I also tried to use $index like:

<div ng-repeat="one in array">
<div isteven-multi-select
    input-model="teams"
    output-model="one[$index].teams"
    button-label="name"
    item-label="name"
    tick-property="ticked"
>
</div>

so the $index variable is different in each step, but it has exactly the same result. From what i learned.. ng-repeat makes its own scope, where the variable one in my example is different for every cycle, so i dont know why it should be a refference problem. As @gr33k01 says, other AngularJS form elements models works perfectly fine there.
If you could show any code that works inside of ng-repeat, it would be great, thanks.

isteven commented 8 years ago

@gr33k01, @veselj43 & all,

Sorry for the late reply, been quite busy with life.

Can you try to to duplicate (deep copy) the input-model into new variables? Reason is; the directive actually modifies your input-model as well. So once you select (or de-select) an item, it will also be reflected in other directives which use the same input-model. Hence other directive will have the same input and output values.

To deep copy, you can simply use angular.copy. Example; let's say you want to repeat 5 directive:

// this is your real data
var originalData = [ { ... }, { ... }, { ... } ]; 

// we duplicate them in array
var duplicatedData = [];
for ( var i = 0; i < 5; i++ ) {
    duplicatedData[ i ] = angular.copy( originalData );
}

Then in your HTML:

<div ng-repeat="data in somethingYouWantToRepeat">
    <div
        isteven-multi-select
        input-model="duplicatedData[ $index ]
        output-model="outputData[ $index ]
        ....
    ></div>
</div>

I have tried it using @gr33k01's plunker and it seems to be working OK. Hopefully it helps.

veselj43 commented 8 years ago

The problem really was in input-model, not output, so by deep copying items for each multiselect solved it. I just forgot about tick-property stored in input object. To be clear, this code works for me:

<div ng-repeat="row in array">
    <div 
        isteven-multi-select
        input-model="selectCopies[$index]"
        output-model="row.selected"
        ...
    ></div>
</div>

This is a solution and I'm glad I can use it, however copying (larger) arrays of data isn't a good way to make it work. If you could store ticked properities somewhere else for every multiselect, it would be a really great feature in my opinion.

Anyways I'd say you can close this. Thank you.

isteven commented 8 years ago

I agree that, indeed, this directive is never a performance champion. I even clearly put this disclaimer in the manual. The worse offender is the grouping logic.

Glad the solution helps. I'll wait for a few days before closing this.

nhobi commented 8 years ago

Thanks for your help on this @isteven @veselj43 and thanks for the great tool. The only thing I'd say is that, since this would seem to be a reasonably common use case, it might be good to add a section for use with ng-repeat in the documentation under Demo:. Maybe it's there and I'm missing it...

isteven commented 8 years ago

@gr33k01 ,

At that time I didn't think much about this in the manual, because actually it's just the usual Javascript object behavior.

Learning from the cases, I guess I need to consider adding this in the manual.