feat(md-chips): added validation for ng-required #11125

fsmeier · 2018-02-16T16:41:22Z

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
[x] Documentation content changes
[ ] Infrastructure changes
[x] Other... Please describe: In my opinion it is more a bug than a feature because this should be a default behavior

What is the current behavior?

ng-required not working with md-chips

Issue Number: #11124

What is the new behavior?

ng-required works with md-chips

Does this PR introduce a breaking change?

[ ] Yes
[x] No (I would say no)

Other information

Pull-Request is mainly a follow-up from #8034. I had the same idea in how to add this but found this not followed/merged pull-request by @Emeegeemee .
So nearly everything of the code got reviewed once by @devversion & @crisbeto . I just changed the naming a bit.

Splaktar

Please add "Fixes #11124" on the last line of your commit (as mentioned in the commit message guidelines).

Splaktar · 2018-02-18T03:38:49Z

src/components/chips/demoValidation/index.html

+      </div>
+    </form>
+  </md-content>
+</div>


Blank line at end of file please.

Splaktar

This is a good start! However, I think that there are a few more finishing touches to put on this feature to make it align with md-input.

Splaktar · 2018-02-18T03:38:55Z

src/components/chips/demoValidation/script.js

+    this.selectedFruit = [];
+    this.selectedVegetables = [];
+  }
+})();


Blank line at end of file please.

Is this fixed @Splaktar ? Because in the code it seems like, but just the one comment of you got hidden, this here not.

Yep, looks good now.

Splaktar · 2018-02-18T03:41:51Z

src/components/chips/demoValidation/index.html

+      </md-chips>
+      <div ng-messages="fruitForm.fruit.$error">
+        <div ng-message="required">At least one fruit is required</div>
+      </div>


These don't show up red because the chips-theme.scss is lacking styles for md-invalid. Can you please review the input-theme.scss and implement something similar for chips as part of this PR?

input.scss also uses a md-input-message-animation class to animate the message in and to set a smaller line height and font size (among other things). Can we please add those styles to chips.scss so that they will be similar when used in a form together?

The error message is showing up even when the input is pristine and not dirty. For input, this isn't the case. Can you please look into this?

Ok, I found out what md-input-container is doing: It adds a special animation-class md-input-messages-animation (https://github.com/angular/material/blob/v1.1.7/src/components/input/input.js#L840) and does some stuff with it. I see two ways here. A: make the demos with md-chips inside md-input-container and assure that this have the same look&feel. B: Add a chips-special-class and add styles there.
But B is not really following the ways it is made atm. ng-messages is just styled inside certain containers. So just A is left, but are chips belonging to md-input-container? - Is there any rule which forbids it?

Yeah, I can see how we wouldn't want to style ng-messages that aren't contained in an AngularJS Material component. I've asked @devversion for some input and created a CodePen to demonstrate how ng-messages are styled with chips today (with md-max-chips) for reference since there is no example in the demos.

fsmeier · 2018-02-19T11:27:37Z

@Splaktar you labeled this PR and the related issue with "bug". Is it a bug or feature in your opinion? - because then I should adjust the commit-message-prefix.

Splaktar · 2018-02-20T01:11:44Z

Yeah, I think you are right. I'll updated this to enhancement. Using feat in the commit message is good.

devversion · 2018-02-20T11:26:21Z

I think just implementing the logic for supporting the required validator is fine here. That's the main intention of the PR, and the design of error messages for chips is more a separate issue in my opinion.

For now, we could just have some custom CSS in the demo and maybe show a small caption under the chips demo that explains that there is no default CSS for error messages yet.

Placing chips inside of the <md-input-container> element makes sense, because the chips can be also considered as an input. Since making the input container more abstract will likely result in breaking changes, I'd vote for another container like <md-form-field>.

The <md-form-field> and <md-input-container> would then share somehow the CSS for the error messages. e.g. using a class like .md-error-messages-container. The actual directives for animations could then be also updated to work in either form-field or input-container.

As you see, this turns out to be a whole new issue, so I'd say that this should be handled in a follow-up.

Splaktar · 2018-02-21T07:57:14Z

@devversion thank you very much. That makes a lot of sense.

@IMM0rtalis can you please look at #11125 (comment) and then add the styles to the demo along with a note about developers needing to style the ng-messages for chips? This note could be similar to the note in the Static Chips Demo.

fsmeier · 2018-02-21T10:49:08Z

@Splaktar I added the note and example-styles and conditional ng-show of error-messages in the demo.
While doing this I ran into problems with the dirty & touched-state not bubbled up from the input so I added a watcher.
In general I figured out that no watcher was deregistered in the chipsController so I also added the destruction of some $watchers for onDestroy.

salimane · 2018-02-22T14:13:10Z

@IMM0rtalis is there a way to use this branch in a project ?

I tried without success :

"devDependencies": {
    "angular-material-source": "git+https://github.com/IMM0rtalis/material.git#c60f055728e1102afa7bb5e9d6e18df021000941"
}

I was using

"devDependencies": {
    "angular-material": "1.1.6"
}

fsmeier · 2018-02-22T14:19:44Z

@salimane not sure, but until the next version with this included is published you could try a hacky way (we had to do it for now)

/**
 * Fix MdChipsController
 *  - fixes problem with not validating required-field (fix in v1.1.8)
 *
 * @see https://github.com/angular/material/pull/11125
 */
(function() {
    function updateMdChipsController(MdChipsCtrl) {

        /**
         * Updates the validity properties for the ngModel.
         */
        MdChipsCtrl.prototype.validateModel = function() {
            this.ngModelCtrl.$setValidity('md-max-chips', !this.hasMaxChipsReached());
            this.ngModelCtrl.$validate(); // rerun any registered validators
        };

        /**
         * Configures the required interactions with the ngModel Controller.
         * Specifically, set {@code this.items} to the {@code NgModelCtrl#$viewVale}.
         * @param ngModelCtrl
         */
        MdChipsCtrl.prototype.configureNgModel = function(ngModelCtrl) {
            this.ngModelCtrl = ngModelCtrl;

            var self = this;

            // in chips the meaning of $isEmpty changes
            ngModelCtrl.$isEmpty = function(value) {
                return !value || value.length === 0;
            };

            ngModelCtrl.$render = function() {
                // model is updated. do something.
                self.items = self.ngModelCtrl.$viewValue;
            };
        };
    }

    var invokes = angular.module('material.components.chips')._invokeQueue,
        i;

    for(i=0; i < invokes.length; ++i) {
        if(invokes[i][0] === "$controllerProvider" && invokes[i][2][0] === "MdChipsCtrl") {
            updateMdChipsController(invokes[i][2][1]);
        }
    }

})();

salimane · 2018-02-23T09:07:15Z

thanks @IMM0rtalis . what is the connection between the current repo and this one https://github.com/angular/bower-material .
do we need the same changes over there ?

Thanks

Splaktar · 2018-02-26T08:37:48Z

@salimane bower-material is the repo where the releases live. This is what gets installed by bower or NPM. Everytime we merge something into master (including making a release), a new, installable build is generated in bower-material and available to Bower and NPM.

salimane · 2018-02-26T08:49:23Z

@Splaktar thanks for reply.

Do you think there is way to have a branch in https://github.com/angular/bower-material that will use the changes in the current pull-request ?

Thanks

Splaktar

This looks good. I just had a couple questions.

Splaktar · 2018-02-26T08:49:19Z

src/components/chips/js/chipsController.js

@@ -170,18 +177,40 @@ MdChipsCtrl.prototype.init = function() {
  var ctrl = this;

  // Set the wrapper ID
-  ctrl.wrapperId = '_md-chips-wrapper-' + ctrl.$mdUtil.nextUid();
+  this.wrapperId = '_md-chips-wrapper-' + this.$mdUtil.nextUid();


What's the benefit of the changes on this line?

Actually I do not see a point in using a helper-var instead of this if this is usable in a place. In my opinion it reduces the readability of code. And if the helper-var for encapsulated inner functions is called ctrl, self or that is in my opinion too much a matter of taste (otherwise I would have replaced ctrl with more widely used helpers in this repository)

Ah OK. 👍

Splaktar · 2018-02-26T08:49:57Z

src/components/chips/js/chipsController.js

+  );
+
+  this.deRegister.push(
+    this.$scope.$watch('$mdChipsCtrl.items', function() {


The comment here is the same as above. What's the reason that this new $watch is needed?

You are just pointing out the replacement of ctrl with this, right? Then see comment above^^

Nope, I'm asking about the two comments "Make sure our input and wrapper have the correct ARIA attributes" and the existing $watchCollection versus the new $watch.

Just added the unwatcher-handling and replaced ctrl with this

Wait... this is duplicated. Fix it asap

devversion · 2018-02-26T17:51:45Z

src/components/chips/js/chipsController.js

+  var $destroyFn;
+  while (($destroyFn = this.deRegister.pop())) {
+    $destroyFn.call(this);
+  }


Why is this made so complicated? What about just this.unregisterFunctions.forEach(fn => fn());?

Because it is ES6, right? Is this repo require es6 ?

ES6 is fine in the build scripts and docs site, but not in the production library code (i.e. anything in src/).

However, you can still use Array.foreach as it's ES5, just not the arrow functions.

Yeah, using the ES5 forEach method should be still a benefit.

If the comparisson-tests are not completely setup wrong I think the while pop is in all cases faster. Have I made a mistake or is there an other reason for using forEach over while-pop?

https://jsperf.com/foreach-vs-while-pop
http://jsben.ch/IGesk

devversion

Other than those two nits. LGTM

devversion · 2018-02-26T17:55:15Z

src/components/chips/js/chipsController.js

+    this.deRegister.push(
+      this.$scope.$watch(
+        function() {
+          return ngModelCtrl.$touched;


You can combine this single watch into two. e.g.

this.$scope.$watch(() => { return { touched: inputNgModelCtrl.$touched, dirty: inputNgModelCtrl.$dirty }; }, XXXX);

But dont I need $watchCollection for this? And then separate afterwards anyway?

Don't think a $watchCollection will work here, because it requires us to watch properties on the $scope. Using $watch and just having a different watch condition should work.

https://stackoverflow.com/questions/17550947/how-to-watch-multiple-variables

If I do as suggested I run in an endless digest-cycle with karma. What speaks against having two simple watcher?

Let's leave it as is, thanks for giving it a try.

fsmeier · 2018-03-08T09:22:42Z

@tinayuangao or @jelbourn or @andrewseguin or @josephperrott or @mmalerba
When I understood correct you are the ones who can/should merge it, right? - Since here is no activity for like 8-10 days I thought it maybe got lost between all the issues. Can sb pls merge it?

Splaktar · 2018-03-09T00:35:51Z

@IMM0rtalis there is no need to mention all of the caretakers. The issue can't be merged until it passes internal presubmit testing.

@tinayuangao started the presubmits on this on 2/27, but I haven't received the results from that yet. I've pinged her for that information.

Thank you for the reminder.

Splaktar · 2018-03-09T00:52:58Z

Presubmits failed. The failures are being investigated. I'll provide more details when they are available.

fsmeier · 2018-03-09T09:17:54Z

Ahh ok. I thought they are the ones who should merge it (https://github.com/angular/material/blob/master/docs/guides/COMMIT_LEVELS.md) and since there was no update... But yeah, they are often not passing I have the feeling :D

Splaktar · 2018-03-10T02:17:07Z

Yep, they will merge it when presubmits get sorted out and any internal failures are fixed (by them). They rotate shifts each week, so mentioning them all isn't ideal. I know who is on shift each week and I work with them to get the merges run through presubmit and merged. This process starts after I apply the pr: merge ready tag.

We recently updated many of the tag descriptions (new GitHub feature) to help make this process a bit more clear. I'll try to document a bit more of the process in the commit levels guide when I have some time.

jelbourn · 2018-03-12T16:51:18Z

Passes presubmit

…)" This reverts commit ba0e9fe.

Splaktar · 2018-04-09T18:59:05Z

This appears to have caused a regression related to adding chips onBlur. We're planning to revert it in #11215 and do more investigation/testing.

This reverts commit ba0e9fe.

* 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.

Fixes #11124 (cherry picked from commit ba0e9fe)

Fixes #11217. Relates to #11125.

* feat(md-chips): added validation for ng-required (#11125) Fixes #11124 (cherry picked from commit ba0e9fe) * fix(chips): allow adding chips on blur when the model is empty Fixes #11217. Relates to #11125.

Fixes angular#11124

* 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.

* feat(md-chips): added validation for ng-required (angular#11125) Fixes angular#11124 (cherry picked from commit ba0e9fe) * fix(chips): allow adding chips on blur when the model is empty Fixes angular#11217. Relates to angular#11125.

Fixes #11124

* 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.

* feat(md-chips): added validation for ng-required (#11125) Fixes #11124 (cherry picked from commit ba0e9fe) * fix(chips): allow adding chips on blur when the model is empty Fixes #11217. Relates to #11125.

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

Splaktar requested review from devversion and Splaktar February 18, 2018 03:15

Splaktar added this to the 1.1.8 milestone Feb 18, 2018

Splaktar self-assigned this Feb 18, 2018

Splaktar added type: bug needs: review This PR is waiting on review from the team P2: required Issues that must be fixed. labels Feb 18, 2018

Splaktar reviewed Feb 18, 2018

View reviewed changes

Splaktar suggested changes Feb 18, 2018

View reviewed changes

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Feb 18, 2018

fsmeier force-pushed the feature/add-chips-require-validation branch 2 times, most recently from 293af81 to 7f4a978 Compare February 19, 2018 11:15

Splaktar added type: enhancement and removed type: bug labels Feb 20, 2018

fsmeier force-pushed the feature/add-chips-require-validation branch 2 times, most recently from a91fc9a to c60f055 Compare February 21, 2018 10:46

Splaktar reviewed Feb 26, 2018

View reviewed changes

devversion reviewed Feb 26, 2018

View reviewed changes

devversion approved these changes Feb 26, 2018

View reviewed changes

Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Feb 27, 2018

Splaktar added the pr: presubmit-failures label Mar 9, 2018

jelbourn merged commit ba0e9fe into angular:master Mar 12, 2018

fsmeier deleted the feature/add-chips-require-validation branch March 12, 2018 16:56

Splaktar removed the pr: presubmit-failures label Mar 12, 2018

fsmeier mentioned this pull request Mar 14, 2018

feat(chips): support use within md-input-container #11155

Open

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

Revert "feat(md-chips): added validation for ng-required (angular#11125…

6b03eb1

…)" This reverts commit ba0e9fe.

Splaktar mentioned this pull request Apr 9, 2018

chips: adding chip on blur no longer working when required is also used #11217

Closed

Splaktar pushed a commit that referenced this pull request Apr 9, 2018

Revert "feat(md-chips): added validation for ng-required (#11125)"

0dcd497

This reverts commit ba0e9fe.

GulajavaMinistudio mentioned this pull request Apr 10, 2018

Update upstream GulajavaMinistudio/material#52

Merged

3 tasks

Splaktar pushed a commit that referenced this pull request Apr 11, 2018

feat(md-chips): added validation for ng-required (#11125)

e3c254b

Fixes #11124 (cherry picked from commit ba0e9fe)

Splaktar added a commit that referenced this pull request Apr 11, 2018

fix(chips): allow adding chips on blur when the model is empty

94126eb

Fixes #11217. Relates to #11125.

Splaktar mentioned this pull request Apr 11, 2018

feat(md-chips): re-added validation for ng-required #11225

Merged

3 tasks

chmelevskij pushed a commit to chmelevskij/material that referenced this pull request Jun 19, 2018

feat(md-chips): added validation for ng-required (angular#11125)

a42f393

Fixes angular#11124

Splaktar pushed a commit that referenced this pull request Jul 31, 2018

feat(md-chips): added validation for ng-required (#11125)

75ffaff

Fixes #11124

feat(md-chips): added validation for ng-required #11125

feat(md-chips): added validation for ng-required #11125

Uh oh!

Conversation

fsmeier commented Feb 16, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Splaktar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsmeier commented Feb 19, 2018

Uh oh!

Splaktar commented Feb 20, 2018

Uh oh!

devversion commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Splaktar commented Feb 21, 2018

Uh oh!

fsmeier commented Feb 21, 2018

Uh oh!

salimane commented Feb 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fsmeier commented Feb 22, 2018

Uh oh!

salimane commented Feb 23, 2018

Uh oh!

Splaktar commented Feb 26, 2018

Uh oh!

salimane commented Feb 26, 2018

Uh oh!

Splaktar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar left a comment •

edited

Loading

devversion commented Feb 20, 2018 •

edited

Loading

salimane commented Feb 22, 2018 •

edited

Loading

Splaktar Feb 26, 2018 •

edited

Loading

fsmeier Feb 27, 2018 •

edited

Loading

fsmeier commented Mar 8, 2018 •

edited

Loading

fsmeier commented Mar 9, 2018 •

edited

Loading