fix(select): make ngOptions support selectAs and trackBy together #9419
Conversation
jeffbcross
added
origin: google
type: bug
needs: review
and removed
origin: google
labels
|
@IgorMinar mind taking a look? |
| * <div class="alert alert-info"> | ||
| * **Note:** The values that are stored on the underlying option elements (i.e. `option.value`) are set | ||
| * to either the indeces of the array (for array data sources) or the property names of | ||
| * the object (for object data sources). |
There was a problem hiding this comment.
this is an implementation detail and not a public api contract. if we want to have this in the docs then we should just reword it to state the public api contract first and then mention the implementation detail as an explanation of why option.value and select.value contain weird stuff.
|
otherwise lgtm |
|
There are some issues when changing from the view that need some work. See this plnkr Select "one" and nothing happens. Select "two" and "one" becomes selected (and the model is 1). If I would decrement both ids, this would work as expected. It seems the logic that assigns the |
|
#9467 may be related to this in some way, I haven't looked into it much --- in any case both sort of revolve around tracking expressions (the bug it tries to fix is not the same issue, but should be related) |
|
Fixed some more issues, and improved viewValue computation. I'm going to do some manual testing and add some more unit tests. |
|
I added more tests and fixed some more cases. @tbosch mind taking a look? |
| }); | ||
|
|
||
| expect(element.val()).toEqual('2'); | ||
| expect(element.val()).toEqual('1'); |
There was a problem hiding this comment.
shouldn't val be 2 since values[1] is {id: 2, name: 'second'} and we track by id?
There was a problem hiding this comment.
The element value is always either the index (for arrays) or key (for objects), even though the model can be different.
There was a problem hiding this comment.
This allows models to be more complex objects. I was confused by this behavior as well, which is why I added a note in the docs about the element value.
| }); | ||
|
|
||
|
|
||
| it('should bind to scope value through selectAs expression while tracking object via track by', function() { |
There was a problem hiding this comment.
new describe: selectAs with trackBy
Maybe add 2 more tests for object data sources
|
More todos aside from commit comments:
|
This commit implements two functions, "isSelected()" and "getViewValue()" to properly compute an option's selected state and the model controller's viewValue respectively. These functions give proper precedence to "track by" and "select as" parts of the ngOptions comprehension expression, which were previously inconsistent and incompatible. Fixes angular#6564
trackBy and selectAs have never worked together, and are fundamentally incompatible since model changes cannot deterministically be reflected back to the view. This change throws an error to help developers better understand this scenario.
| * trackBy is always applied to `value`, with purpose to preserve the selection, | ||
| (to `item` in this case) | ||
| * To calculate whether an item is selected, `ngOptions` does the following: | ||
| 1. apply `trackBy` to the values in the array, e.g. |
|
Otherwise LGTM. |
|
Travis failed because of a jshint error... |
jeffbcross
force-pushed
the
master
branch
2 times, most recently
from
abdaab7
to
30996f8
Compare
|
Closed at 30996f8 |
This commit implements a function, "getSelected()", that takes the
selectAs expression into account when determining if a particular
option is selected or not. Previously, the selectAs part of the
ngOptions comprehension expression was being ignored.
Fixes #6564