-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix(chips): regression where chips model gets out of sync with view #11310
Conversation
31fb221
to
0cc804d
Compare
* @param {boolean=} skipValidation true to skip calling validateModel() | ||
*/ | ||
MdChipsCtrl.prototype.updateNgModel = function(skipValidation) { | ||
this.ngModelCtrl.$setDirty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call $setViewValue()
? This will break expected ngModel
behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With what value? Calling $setViewValue(this.items.slice())
is what caused these regressions (issues linked in OP). Calling $setViewValue(this.items)
doesn't trigger ngChange
, but I guess I could try that again now that we're doing it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.ngModelCtrl.$setViewValue(this.items);
does seem to work now with the manual triggering of ngChange
. Will update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. So, yeah, calling $setViewValue(this.items)
might not work (since it uses ===
to compare to previous value) and thus will not perform the expected operations (update classes, validate, call $viewChangeListeners
).
As a workaround, maybe you could call $viewChangeListeners
yourself, which I think it a better approach than manually calling ngChange
(relevant AngularJS code):
angular.forEach(ngModelCtrl.$viewChangeListeners, function(listener) {
try {
listener();
} catch (e) {
$exceptionHandler(e);
}
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thank you. That seems to work and resolves the argument naming issue.
this.validateModel(); | ||
} | ||
if (this.ngChange) { | ||
this.ngChange(this.items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, this will not be called correctly. Since you are defining a scope binding (via ngChange: '&'
), you need to pass an object that maps arguments to values. For example, if you have ng-change="onChange(items)"
, then you need to call it with this.ngChange({items: this.items})
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this.ngChange({items: this.items})
the solution?
Does that work if they do something like ng-change=onChange(myItems)
?
Since we're using controllerAs
and bindToController: true
, is there a better way to hook up/into ngChange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key has to be the same as what they used as variable name. Essentially, a &
binding allows you to evaluate an expression on the parent scope (of an isolate scope) and pass certain data. For security reasons, AngularJS creates and passes a wrapper to the isolate scope. So, the isolate scope does not get direct access to the expression (most often a method). What it gets is a wrapper function that takes a map of argument names to values and calls the expression on the parent scope with these values.
For example, having a parentMethod: '&'
binding and using the component as parent-method="doStuff(arg1, arg2)"
, allows the component's isolate scope to call this.parentMethod({arg1: 'value1', arg2: 'value2'})
, which will eventually call doStuff('value1', 'value2')
on the parent scope.
So, you need to know what arguments are used in the expression.
Usually, we don't have this issue with ngChange
, because there is no isolate scope. In cases like this (where you have an isolate scope and you can't just .$eval(attrs.ngChange)
), a common approach is to specify (i.e. document) that in the ngChange
expression the chips will be available as $chips
or $items
(or whatever name you choose).
Then, people can use ng-change="onChipsChange($chips)"
and you can call it with this.ngChange({$chips: this.items})
(and their onChipsChange()
method will be called with this.items
as argument).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the explanation! That is quite helpful.
It looks like this is not needed due to the suggested change in #11310 (comment).
0cc804d
to
66c0ccf
Compare
66c0ccf
to
7d6db21
Compare
manually call `ngChange` as it won't be triggered by `ngModel` for chips add `ng-change` test to contact-chips refinements to chips and contact-chips docs remove references to `filter-selected` on contact-chips it was disabled 2 years ago remove use of `angular.lowercase` which is removed in AngularJS 1.7 Fixes #11304. Fixes #11301.
7d6db21
to
fd225a1
Compare
I confirmed that that internal app w/ the chips problem works with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…11310) manually call `ngChange` as it won't be triggered by `ngModel` for chips add `ng-change` test to contact-chips refinements to chips and contact-chips docs remove references to `filter-selected` on contact-chips it was disabled 2 years ago remove use of `angular.lowercase` which is removed in AngularJS 1.7 Fixes #11304. Fixes #11301.
PR Checklist
Please check that your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
There was a breaking change in 1.1.9 related to the enhancement to support
ng-change
onmd-chips
. This was introduced in #11166 where the passed in model and theitems
Array is replaced viaArray.slice
. This appears to have led to the issues listed below and may be related to #11218 as well.Issue Number:
Fixes #11304. Fixes #11301.
What is the new behavior?
ngChange
as it won't be triggered byngModel
for chipsng-change
test to contact-chipsfilter-selected
on contact-chipsangular.lowercase
which is removed in AngularJS 1.7Does this PR introduce a breaking change?
Other information