fix(select): assign result of track exp to element value #9726
Conversation
Fixes a regression where the option/select values would always be set to the key or index of a value within the corresponding collection. Prior to some 1.3.0 refactoring, the result of the track expression would be bound to the value, but this behavior was not documented or explicitly tested. A cache was added in order to improve performance getting the associated value for a given track expression. This commit adds one explicit test for this behavior, and changes several other trackBy tests to reflect the desired behavior as well.
|
CLA verified |
| @@ -726,6 +726,19 @@ describe('select', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| it('should use the tracked expression as option value', function () { | |||
There was a problem hiding this comment.
2 space indentation plz --- this is angular not chromium :p
|
part of this fix looks like a revert of test case changes added in 1.3 --- it would be better to document those as reverts --- but otherwise I think it's okay with the indentation nit fixed |
|
My fault, I've fixed the indentation. |
|
I just meant that doing an actual |
|
@jeffbcross can I assign this to you since you've been working on this lately? it basically looks good to me |
|
LGTM. I added a test from my PR and squashed commits. Restarted flaky Travis job to make sure we're green before merging. |
|
oh yeah --- @hzoo added some extra jscs strictness a while ago, so this is now breaking the style rules a tiny bit. |
| @@ -556,9 +561,14 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { | |||
|
|
|||
| // doing displayFn(scope, locals) || '' overwrites zero values | |||
| label = isDefined(label) ? label : ''; | |||
| optionId = trackFn ? trackFn(scope, locals) : (keyName ? keys[index] : index); | |||
| if (trackFn){ | |||
There was a problem hiding this comment.
space after (trackFn) before {
|
(these are nits, can be fixed up during landing if it looks good) |
|
Thanks @caitp I'll run Travis again with those fixes |
|
Landed! 4b4098b Thanks for the PR @juangabreil (and nice to meet you last week) |
|
My pleasure! Nice to meet you too @jeffbcross! |
Fixes a regression where the option/select values would always be set to
the key or index of a value within the corresponding collection. Prior to
some 1.3.0 refactoring, the result of the track expression would be bound
to the value, but this behavior was not documented or explicitly tested. A
cache was added in order to improve performance getting the associated
value for a given track expression.
This commit adds one explicit test for this behavior, and changes several
other trackBy tests to reflect the desired behavior as well.
Fixes #9592 and it's related to PR #9718 (@jeffbcross)