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

Add possibility to find value according to a property of the model #256

Merged
merged 3 commits into from
Jan 22, 2015

Conversation

JeffGreat
Copy link
Contributor

I needed that feature to preload existing model objects that are not in the source list.
The comparison is made upon the property setted via the "track by" of the repeater.

I needed that feature to preload existing model objects that are not in the source list.
The comparison is made upon the property setted via the "track by" of the repeater.
@ITman1
Copy link

ITman1 commented Oct 9, 2014

Nice job, works well, thanks! Fixed my pain of using unsupported "track by".

@dimirc
Copy link
Contributor

dimirc commented Oct 10, 2014

Can you explain me the use case for this? Maybe we already can do if with current implementation

@ITman1
Copy link

ITman1 commented Oct 10, 2014

The problem was that I fetched from server instances of choices and then instances of the selected items. These instances were different, so it did not match anything. By this fix instances can be now tracked by 'id'. Maybe this is already supported, but it did not work for me before this fix.

@ITman1
Copy link

ITman1 commented Oct 10, 2014

@JeffGreat
Copy link
Contributor Author

Exactly, the problem I had was the same: I get instance from server, those instances are ng-resource and that's why it didn't match anything in my choice list. Therefore track by can be useful in that way.

@masscrx
Copy link

masscrx commented Oct 10, 2014

ITman1: Where is the difference ? I can't find where code is changed

@ITman1
Copy link

ITman1 commented Oct 10, 2014

Only in the initialization of the selection, so look for $scope.multipleDemo.selectedPeople. In the first example there are identical instances. In the second example instances are new, but selected items are semantically the same.

@dimirc
Copy link
Contributor

dimirc commented Oct 29, 2014

@JeffGreat sorry the delay on the review, can you create a simple test for this feature?

@JeffGreat
Copy link
Contributor Author

Sorry dimirc I didn't see you comment. I'll try to make test quite soon.

@JeffGreat
Copy link
Contributor Author

dimirc, I've just change the test spec file but since it became quite old I wonder if I can send you a pull request. Let me know.

Arf, build failed

@amcdnl
Copy link
Contributor

amcdnl commented Dec 22, 2014

@JeffGreat - I love the fix to the object comparison issue. If you can do a few things for me I'll merge it up:

  • Re-base with latest.
  • Would love to see another object in the list to make the test a bit more complex and show it doesn't match the second.
  • Add the condition for the single value. The fact that it works now is just a pure fluke, if anyone changes that logic it will fail too. Honestly, I'd love to change this logic to be a little more clear.

@dimirc - This is a valid problem; it happens when you have prototype objects that ARE the same but the comparison == doesn't return true. The track by expression allows you specify the 'key' to match on.

Its also worth noting if you did angular.equals it would work since it does a deep compare. Its not performant at all though so a track by solution would probably still be a better fix.

@PowerKiKi
Copy link

This is very interesting. If we can merge, I believe it would allow us to close #333, #414, #332.

@JeffGreat
Copy link
Contributor Author

Ok i'll try to update the fix asap since I'm a bit busy. Hope to post something in a few days (during the holidays :) )

@JeffGreat
Copy link
Contributor Author

@amcdnl - I've rebased with latest and pushed it on my fork.
What do you mean by "Add the condition for the single value"?

@abobwhite
Copy link

Hey guys - any movement on this? I'm surprised this didn't work from the start. As such, I am developing with the component and am stuck until the fix is merged or I hack up my own solution.

Just curious if it's planned to be merged soon.

@brianfeister
Copy link

@amcdnl can you and @JeffGreat sort out your request here?

@JeffGreat
Copy link
Contributor Author

I've updated the pull request, all the test passed so it just need to be merged now. @dimirc can you merge?

@brianfeister
Copy link

@JeffGreat - I can merge it but it sounded like you and @amcdnl still hadn't resolved one of his bullet points.

@Flightfreak
Copy link

👍

@JeffGreat
Copy link
Contributor Author

@brianfeister you can merge since the last bullet point seems to be an improvment. On my side the feature work as is and can help others.
when @dimirc will explain me what is the last bullet I'll do another pull request.

@burzum
Copy link

burzum commented Jan 21, 2015

👍 Yes, please merge this ASAP, I've just wasted a good amount of time just to figure out this is not working with ui-select. 😞

@amcdnl
Copy link
Contributor

amcdnl commented Jan 21, 2015

@JeffGreat Just apply the track by change for single selections. Currently you only have it for multi

amcdnl added a commit that referenced this pull request Jan 22, 2015
Add possibility to find value according to a property of the model
@amcdnl amcdnl merged commit 28466ab into angular-ui:master Jan 22, 2015
@amcdnl
Copy link
Contributor

amcdnl commented Jan 22, 2015

Merged ... I'll fix the other issue.

I added docs too: https://github.com/angular-ui/ui-select/wiki/ui-select#multi-select-with-complex-objects

@hsrobmln
Copy link

@JeffGreat Thank you so much for fixing this! I was going insane thinking I was doing something wrong. Exact same situation, I have a list of possibilities loaded from the server, and the selections are pre-populated on a different object, so they weren't matching. This solves the problem I've spent the last 3 hours on.

By the way, the merged PR is not updated in the dist/ folder... I had to npm install && gulp to get this working. So make sure you run gulp to rebuild the dist/select.js file

@MarkRBM MarkRBM mentioned this pull request Jan 28, 2015
@MarkRBM
Copy link

MarkRBM commented Jan 28, 2015

Can a tag be created that contains this update? 0.9.6 dosn't seem to have it

@jukkasi
Copy link

jukkasi commented Jan 31, 2015

I cannot make this work in my use case using multiselect, options are fetched async with refresh="fetch($select.search) from HTTP backend

The problem is that selecting works fine and I can save to backend, but when backend returns results, UI is not updated accordingly.

My fetch() does not return all the options with empty search parameter but only limited list where previously selected value is or isn't included. The matching logic in this PR does not help since empty fetch is performed in some point and matching is performed against options where the selected value does not exist.

So my fetch() is really dynamic and the resulting options depend on given search parameter. I can easily fix this so that fetch() with empty search term would return all the possible options, but this does not make sense in performance point of view. To be more specific, my options filtering is done on server-side.

Single-select (without multiple) works just fine and as expected in this case so I don't know why multiple should be any different.

Any hints for this scenario?

@MarkRBM
Copy link

MarkRBM commented Feb 3, 2015

@jukkasi I don't think this fix has been released yet, it is in master but it dosn't seem to be in the 0.9.6 build that is the latest on bower

@amcdnl is this correct? can we get a build done?

@jukkasi
Copy link

jukkasi commented Feb 3, 2015

@ir1sh I understand it's not released and tested as local patch. I believe the problems with multiple are much bigger than this PR adresses.

@MarkRBM
Copy link

MarkRBM commented Feb 3, 2015

I agree for one I don't understand why the object has to be in the choices object to be added to the selected

edit1: My use case is this;

-Users can add guests to a reservation. these guests can be from a list of user objects that are relatively complicated
-Users can also add custom guests in the form of an email address
This all works fine so far. The problem is when the server returns the list of guests it includes both the user objects and the email address objects. I add the track by property and the guests that are in the User list get pre populated but no matter what I do to I cant get it to prepopulate the email address only objects

edit2: this is with this pull request included

@jukkasi
Copy link

jukkasi commented Feb 3, 2015

Exactly. And other major issue reported elsewhere by many is that setting the model does not update view accordingly, all of this is more or less related. This PR is referenced from many issue reports but actually I don't think it helps for any of those..

@MarkRBM
Copy link

MarkRBM commented Feb 3, 2015

@JeffGreat if I change the track by to person.email instead of person.name in your test it still passes. should it fail?

@JeffGreat
Copy link
Contributor Author

it should fail. Actually since the email is different there is no reason to get a selected value. May be the assertion is not right.

@MarkRBM
Copy link

MarkRBM commented Feb 3, 2015

even if I remove the track by in el it still passes

@MarkRBM
Copy link

MarkRBM commented Feb 3, 2015

works correctly in demo-multi-select.html

@hkothari
Copy link

hkothari commented Feb 4, 2015

@dimirc @amcdnl : to @ir1sh 's point above, would it be possible to get a 0.9.7 tag with this fix? It seems like there is sufficient desire from users to take advantage of "track by" support enabled by this.

@zukasmichael
Copy link

hi guys ? how to get this patch or tag preloading working ? spent 4 hours yesterday on trying many workarounds and i'm about going to select2

@brianfeister
Copy link

@amcdnl - hoping to get a new release with some bugfixes out soon. Should I assume those are 0.9.7 and this is 0.10.0?

@zukasmichael
Copy link

i have tried 0.9.7 - still can't preload values if they are not in ui-select-choices http://plnkr.co/edit/o0pAujq9u4gCq2X96WEZ?p=preview

@brianfeister
Copy link

Next release. 0.9.8

Sent from my iPhone

On Feb 14, 2015, at 12:34 PM, zukasmichael notifications@github.com wrote:

i have tried 0.9.7 - still can't preload values if they are not in ui-select-choices http://plnkr.co/edit/o0pAujq9u4gCq2X96WEZ?p=preview


Reply to this email directly or view it on GitHub.

brianfeister pushed a commit that referenced this pull request Feb 16, 2015
@dimirc dimirc removed this from the 0.10.x milestone Feb 18, 2015
@zhendong590
Copy link

👍

@RensBonnez
Copy link

Does the track by work already for single selection? Can't make it work.

@bademux
Copy link

bademux commented Oct 14, 2015

doesn't work without proxy object multipleDemo $scope.multipleDemo = {}; on v 0.12
seee for mo infohttp://plnkr.co/edit/o0pAujq9u4gCq2X96WEZ?p=preview

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet