fix(select): use $viewValue instead of $modelValue #8937
Conversation
|
isn't it technically possible for two options to have the same viewValue? --- I guess it's probably possible for them to have the same modelValue though too.. hm |
|
Yes, but then they will also have the same modelValue, the behavior will be consistent with how it works today. In fact, the way I see it, the code we had before this change effectively assumed that BTW, do you think I should add some tests to show that those things work now? |
How would they have the same model value the same way they do today? Don't we use their value binding from ng-options, or otherwise ng-value for this instead? I mean, if you have two optgroups with like, indexed matrix values for values, but two rows have the same view values, that kinda breaks the optgroups here (but not before), does it not? |
|
I am not very familiar with this directive, in fact today is the first time I looked at |
|
@caitp do you mind sending a plunkr example of the optgroup case you mentioned? I can't see how it might be effected by this change. I'm probably missing something... |
|
I have no idea if it would break this or not, lets verify that it doesnt |
|
You mean something like this? http://plnkr.co/edit/E2Zg4ueMp3FbIJuLTTGL?p=preview |
|
@petebacondarwin - what do you think about this? does it make any sense? |
|
@shahata - I think this is probably the right thing to do. The select directive was originally written a long long time ago - before NgModelController even existed and its code looked so complicated people steered clear of refactoring it as long as it appeared to work. So originally the select directive completely subverted how NgModelController does stuff (i.e. it didn't care about $viewValue, $parsers, $formatters (and now $validators) etc. It just watched the model expression on the scope and sorted itself out. Without actually running this code, it seems like it should work for me. And in fact as @shahata points out will enable much better integration with $parsers et al. I think adding in such tests would be a very good thing to do. I say let's get this in. |
|
👍 Added some tests to show that |
shahata
changed the title
| @@ -1437,6 +1437,130 @@ describe('select', function() { | |||
| expect(scope.value).toBe(false); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('ngModelCtrl', function() { | |||
| it('should support $parsers', function() { | |||
There was a problem hiding this comment.
nit: these test names are not explaining what is actually being tested, please fix
|
@shahata there does appear to be a bug when it comes to duplicate options (as demonstrated by your plunker, but also verified by rebuilding with your patch). I guess thats a bug with ngOptions though |
|
Yeah, it is unrelated to this issue. I can open a separate PR for it, but I'm not sure we should fix it... |
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa to
fd2d6c0
Compare
| 'ng-options': 'item for item in [\'first\', \'second\', \'third\']', | ||
| }); | ||
|
|
||
| scope.form.select.$asyncValidators.fail = function() { |
There was a problem hiding this comment.
fail? just call it asyc in this and previous test
There was a problem hiding this comment.
yeah, the first fail was correct, the others are copy & paste ;). fixed!
|
otherwise this looks good to me |
|
oops, this is broken after rebasing with 2bcd02d but I'm too tired to look into it now, will take a look tomorrow |
|
Hey @shahata any luck with the rebase? |
|
@jeffbcross - yes, had a few urgent things at work, but now I'm on it. This commit is breaking because of a bug in 2bcd02d |
|
Okay, build passes now. This is the issue I mentioned - #9418 |
|
Landed, thanks! f717416 |
Closes #8929