feat(chips): trigger ng-change on chip addition/removal #11166

chenlijun99 · 2018-03-13T14:40:53Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

ng-change is not supported for md-chips or md-contact-chips.

Issue Number:
#11161 #3857

What is the new behavior?

ng-change is supported on md-chips. It will be triggered on chip addition/removal.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Closes #11161 and #3857

Splaktar · 2018-03-13T19:12:37Z

Sorry for more commit message confusion. Closes #11161 and #3857 needs to be Closes #11161. Closes #3857 for the GitHub parsing to work properly.

Splaktar

Thank you for your contribution!

Please add the appropriate docs (as comments) in the chipsDirective.js and contactChipsDirective.js files for supporting ng-change.
Can you please add the use of ng-change to the current Contact Chips demo?
Can you please add a demo for ng-change that covers normal Chips?
Can you please add a test to the contact-chips.spec.jsfile?

chenlijun99 · 2018-03-13T20:22:50Z

@Splaktar Why do we need a test of ng-change for md-contact-chips? Since md-contact-chips is just a wrapper of md-chips, I think it's redudant to test on both the components.

Splaktar · 2018-03-14T01:36:21Z

OK, that test may not be needed, as long as we can see it working in the demo. It still seems somewhat useful to have though to make sure that some change to md-contact-chips does not break the ng-change functionality.

Splaktar

Updated changes LGTM, but I still need to do some manual testing.

fsmeier

I just added two more comments with rly small stuff but to increase clean code in my opinion, but i like the PR overall very much!

fsmeier · 2018-03-14T09:08:33Z

src/components/chips/js/contactChipsDirective.js

@@ -122,6 +124,7 @@ function MdContactChips($mdTheming, $mdUtil) {
      contactImage: '@mdContactImage',
      contactEmail: '@mdContactEmail',
      contacts: '=ngModel',
+      ngChange: "&",


Why double quote here when all the other bindings have single quote?

Opss, it just a matter of habit. In the project I'm working on we use double quotes. I'll fix.

In the meantime I've fixed this.

fsmeier · 2018-03-14T09:10:25Z

src/components/chips/js/chipsController.js

@@ -329,6 +329,7 @@ MdChipsCtrl.prototype.updateChipContents = function(chipIndex, chipContents){
  if(chipIndex >= 0 && chipIndex < this.items.length) {
    this.items[chipIndex] = chipContents;
    this.ngModelCtrl.$setDirty();
+    this.ngModelCtrl.$setViewValue(this.items.slice());


Since this line is repeated 3-times - why not wrap it into a helper-function similar to validateModel() ?

It seems there is already a function called validateModel and I think that updateModel describes better what does this statement do.

But do we really need an helper function for just a single line of instruction?

On the other hand, if you meant to wrap into a function the two lines that are repeated

this.ngModelCtrl.$setDirty(); this.ngModelCtrl.$setViewValue(this.items.slice());

then I think we can remove this.ngModelCtrl.$setDirty(), since AFAIK $setViewValue() should already set the model as $dirty

oh, I maybe choosed the wrong words. I meant something like the validateModel(), not name it this way.
But it is 3-times calling a function $setViewValue and chooses the a copy (slice()) from the inner value-cache (this.items). Maybe there is a small change there so the developer has to update now 3-lines instead of one.
This is just an suggestion to wrap this in an extra function because I like to reduce repeated code and make it easier to maintain. But every developer has different workflows.

PS.: if this helper-function could also reduce other duplications it is even better.

chenlijun99 · 2018-03-14T14:29:03Z

@IMM0rtalis
How do you like the changes I made?
In order to make the unit tests pass, I needed to call this.validateModel in updateNgModel as well, which
is kinda redudant since by calling this.ngModelCtrl.$setViewValue the validation is automatically run.
I've added a TODO, since the possible minor refactor would be better done in a different pull-request.

Splaktar · 2018-03-14T23:29:04Z

src/components/chips/demoBasicUsage/script.js

@@ -39,5 +40,9 @@
        type: 'unknown'
      };
    };
+
+    self.onModelChange = function(newModel) {
+      alert("The model has changed");


Can you please use single quotes here?

Splaktar · 2018-03-14T23:29:16Z

src/components/chips/demoContactChips/script.js

@@ -84,6 +85,10 @@

    }

+    function onModelChange(model) {
+      alert("The model has changed");


Can you please use single quotes here?

* Add test of `ng-change` for `md-chips` * Add docs regarding `ng-change` for `md-chips` and `md-contact-chips` * Add demo for ng-change on `md-chips` * Add demo for ng-change on `md-contact-chips` Closes angular#11161 Closes angular#3857

Splaktar · 2018-03-22T08:19:57Z

Testing done. Submitting to presubmit.

…lar#11166)" This reverts commit 19da42d.

Splaktar · 2018-04-07T07:39:11Z

I'm not sure what the details are yet, but there may be some issue with these changes that may require reverting them. Just a heads up. More details may be available in #11214 at some point.

* Revert "feat(chips): trigger ng-change on chip addition/removal (#11166)" This reverts commit 19da42d. * Revert "feat(md-chips): added validation for ng-required (#11125)" This reverts commit ba0e9fe.

Splaktar · 2018-05-30T01:18:44Z

src/components/chips/js/chipsController.js

@@ -584,16 +582,21 @@ MdChipsCtrl.prototype.validateModel = function() {
  this.ngModelCtrl.$validate(); // rerun any registered validators
 };

+MdChipsCtrl.prototype.updateNgModel = function() {
+  this.ngModelCtrl.$setViewValue(this.items.slice());


This line seems to be related to #11304 and #11301.

* Add test of `ng-change` for `md-chips` * Add docs regarding `ng-change` for `md-chips` and `md-contact-chips` * Add demo for ng-change on `md-chips` * Add demo for ng-change on `md-contact-chips` Closes angular#11161 Closes angular#3857

* Revert "feat(chips): trigger ng-change on chip addition/removal (angular#11166)" This reverts commit 19da42d. * Revert "feat(md-chips): added validation for ng-required (angular#11125)" This reverts commit ba0e9fe.

* Add test of `ng-change` for `md-chips` * Add docs regarding `ng-change` for `md-chips` and `md-contact-chips` * Add demo for ng-change on `md-chips` * Add demo for ng-change on `md-contact-chips` Closes #11161 Closes #3857

* Revert "feat(chips): trigger ng-change on chip addition/removal (#11166)" This reverts commit 19da42d. * Revert "feat(md-chips): added validation for ng-required (#11125)" This reverts commit ba0e9fe.

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Mar 13, 2018

FjVillar mentioned this pull request Mar 13, 2018

feat(sidenav): add disable click and escape event #11165

Merged

3 tasks

Splaktar self-requested a review March 13, 2018 19:10

Splaktar self-assigned this Mar 13, 2018

Splaktar added the type: enhancement label Mar 13, 2018

chenlijun99 force-pushed the fix-issue-11161 branch 2 times, most recently from 72c6242 to 12134e2 Compare March 13, 2018 19:15

Splaktar suggested changes Mar 13, 2018

View reviewed changes

Splaktar added this to the 1.1.9 milestone Mar 13, 2018

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P4: minor Minor issues. May not be fixed without community contributions. needs: unit tests This PR needs unit tests to cover the changes being proposed labels Mar 13, 2018

chenlijun99 force-pushed the fix-issue-11161 branch from 12134e2 to ba5e72e Compare March 13, 2018 20:21

Splaktar approved these changes Mar 14, 2018

View reviewed changes

fsmeier reviewed Mar 14, 2018

View reviewed changes

chenlijun99 force-pushed the fix-issue-11161 branch 2 times, most recently from 36ec784 to 4928ed3 Compare March 14, 2018 14:15

Splaktar reviewed Mar 14, 2018

View reviewed changes

chenlijun99 force-pushed the fix-issue-11161 branch from 4928ed3 to 73bfa8a Compare March 15, 2018 08:02

Splaktar removed the needs: manual testing This issue or PR needs to have some manual testing and verification done label Mar 22, 2018

Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 22, 2018

andrewseguin merged commit 19da42d into angular:master Mar 23, 2018

jelbourn added a commit to jelbourn/material that referenced this pull request Apr 6, 2018

Revert "feat(chips): trigger ng-change on chip addition/removal (angu…

85ff8df

…lar#11166)" This reverts commit 19da42d.

Splaktar mentioned this pull request Apr 9, 2018

autocomplete(chips): deleting chips breaks ability to add or render new chips #11218

Closed

GulajavaMinistudio mentioned this pull request Apr 10, 2018

Update upstream GulajavaMinistudio/material#52

Merged

3 tasks

Splaktar mentioned this pull request Apr 16, 2018

feat(chips): trigger ng-change on chip addition/removal #11237

Merged

3 tasks

This was referenced May 30, 2018

chips: when the model(array) is modified the chips view is not updated #11304

Closed

chips: ng-change with read-only array-model causes problems #11301

Closed

Splaktar reviewed May 30, 2018

View reviewed changes

Splaktar mentioned this pull request Jun 1, 2018

fix(chips): regression where chips model gets out of sync with view #11310

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chips): trigger ng-change on chip addition/removal #11166

feat(chips): trigger ng-change on chip addition/removal #11166

chenlijun99 commented Mar 13, 2018 •

edited by Splaktar

Loading

Splaktar commented Mar 13, 2018

Splaktar left a comment •

edited

Loading

chenlijun99 commented Mar 13, 2018

Splaktar commented Mar 14, 2018

Splaktar left a comment •

edited

Loading

fsmeier left a comment

fsmeier Mar 14, 2018

chenlijun99 Mar 14, 2018

chenlijun99 Mar 14, 2018

fsmeier Mar 14, 2018

chenlijun99 Mar 14, 2018

chenlijun99 Mar 14, 2018

fsmeier Mar 14, 2018

chenlijun99 commented Mar 14, 2018

Splaktar Mar 14, 2018

chenlijun99 Mar 15, 2018

Splaktar Mar 14, 2018

chenlijun99 Mar 15, 2018

Splaktar commented Mar 22, 2018

Splaktar commented Apr 7, 2018

Splaktar May 30, 2018

feat(chips): trigger ng-change on chip addition/removal #11166

feat(chips): trigger ng-change on chip addition/removal #11166

Conversation

chenlijun99 commented Mar 13, 2018 • edited by Splaktar Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Splaktar commented Mar 13, 2018

Splaktar left a comment • edited Loading

Choose a reason for hiding this comment

chenlijun99 commented Mar 13, 2018

Splaktar commented Mar 14, 2018

Splaktar left a comment • edited Loading

Choose a reason for hiding this comment

fsmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenlijun99 commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Splaktar commented Mar 22, 2018

Splaktar commented Apr 7, 2018

Choose a reason for hiding this comment

chenlijun99 commented Mar 13, 2018 •

edited by Splaktar

Loading

Splaktar left a comment •

edited

Loading

Splaktar left a comment •

edited

Loading