fix(ngChecked): ensure that ngChecked doesn't interfere with ngModel #10664
Conversation
| linkFn = function(scope, element, attr) { | ||
| // ensuring ngChecked doesn't interfere with ngModel when both are set on the same input | ||
| // - https://github.com/angular/angular.js/issues/10662 | ||
| !attr.ngModel && defaultLinkFn(scope, element, attr); |
There was a problem hiding this comment.
ngChecked only really breaks things if the ngModel attribute value is the same as the ngChecked attribute value
So more like attr.ngModel !== attr[normalized] && ...
There was a problem hiding this comment.
However, for code style, please write it like this:
if (attr.ngModel !== attr[normalized) {
defaultLinkFn(scope, element, attr);
}There was a problem hiding this comment.
@caitp Done. Please let me know if something else requires adjustment.
LeonardoBraga
force-pushed
the
fix-ng-checked-10662
branch
from
d5fc6ec to
2368d01
Compare
| linkFn = function(scope, element, attr) { | ||
| // ensuring ngChecked doesn't interfere with ngModel when both are set on the same input | ||
| // - https://github.com/angular/angular.js/issues/10662 | ||
| if (attr.ngModel !== attr[normalized) { |
There was a problem hiding this comment.
kinda feel like special casing different attributes in here is kind of gross, too --- but we can worry about that later
There was a problem hiding this comment.
:-$ - sorry, it's fixed now.
There was a problem hiding this comment.
kinda feel like special casing different attributes in here is kind of gross, too --- but we can worry about that later
Sure. Just ping if you want me to consider that in another PR.
LeonardoBraga
force-pushed
the
fix-ng-checked-10662
branch
from
2368d01 to
ceb1471
Compare
|
lgtm |
|
@Narretz am I missing anything? could you give this a second look before merging thanks |
| } | ||
|
|
||
| var normalized = directiveNormalize('ng-' + attrName), | ||
| linkFn = defaultLinkFn; |
There was a problem hiding this comment.
Can you explicitly declare the linkFn variable? I think we prefer this now.
There was a problem hiding this comment.
So linkFn would have it's own var? I'm asking because it differs with other places I saw in the code. Just let me know if that's indeed the case and I'll change it.
There was a problem hiding this comment.
Yes, that's what I mean. It's a minor thing - but recent code changes favour this version.
There was a problem hiding this comment.
agree with @Narretz, we prefer to give each variable it's own var, it's a bit easier than dealing with a huge comma-separated list (see compile.js for examples :x) =)
There was a problem hiding this comment.
Not a problem! :-) Thanks for the instructions and I'll be updating the PR soon.
|
Apart from these small nitpicks, this looks good to me. I also didn't know we had this directive ... |
LeonardoBraga
force-pushed
the
fix-ng-checked-10662
branch
from
ceb1471 to
ef3a4f2
Compare
|
lgtm, will merge when green |
|
@caitp Travis seems flaky again. Should I close + reopen to force a re-test? |
|
no need, restarted manually |
|
Could you please restart again? :-/ |
|
it's just browserstack having issues, it's not failing in a consistent way, so I think it's fine. chock it up to flakes |
|
@petebacondarwin I think this is a good candidate for cherry-picking into 1.3, but can you make the call on that? |
Fixes #10662