feat(mdMaxlength): support use with required and ng-trim #11136

Splaktar · 2018-02-27T04:28:43Z

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?

When md-maxlength is used on an input that has the required attribute set, the required validation will accept blank spaces in the input as valid by default. Trying to override this with ng-trim="true" is ignored.

Issue Number:
Fixes #10082
Fixes #10216
Relates to #5321

What is the new behavior?

By default, blank spaces in required inputs cause an invalid state. If this is undesirable, it can be overridden by adding ng-trim="false" to the input.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

This is based on an abandoned PR (#10083).

googlebot · 2018-02-27T04:28:46Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Splaktar · 2018-02-27T04:31:44Z

CLA consent here: #10083 (comment)

Splaktar · 2018-02-27T04:38:59Z

I can't get the test to pass which is supposed to verify this functionality. In manual testing (demos), I can clearly see the functionality working as expected. But I can't get the required error to trigger in the test.

Splaktar · 2018-03-13T07:29:53Z

The previous test issues have been resolved. Now the only failing tests have to do with AngularJS 1.5.x...

jelbourn

LGTM

Splaktar · 2018-03-13T18:58:21Z

Thanks a ton @gkalpak! That's exactly the kind of feedback that I was looking for! I'll re-visit and make changes.

gkalpak · 2018-03-13T19:18:48Z

👍 Feel free to ping me for another review 😃

Adding ng-trim="false" to an input will cause the 'required' state to be fulfilled by empty spaces, essentially allowing a user to put in no content and pass required validation. Allowing angular to trim, and trimming manually in the renderCharCount function works better (IMO).

Splaktar · 2018-04-19T23:42:21Z

@gkalpak can you please take another look?

viewValue did not have trim already applied in my debugging.
I believe that I was able to properly override the ngModelCtrl.$isEmpty() function instead of the required validator.
The last few checks that didn't work for 0 should be fixed.

gkalpak

A couple of nits.
LGTM otherwise 👍

gkalpak · 2018-04-25T17:45:26Z

src/components/input/input.js

+ elementVal = '';
+ }
+ elementVal = ngTrim && !isPasswordInput && angular.isString(elementVal) ? elementVal.trim() : elementVal;
+ return String(elementVal).length === 0;


Nit: You can make a re-usable function for calculating the length of a value (which is used in the validator, $isEmpty() and renderCharCount()):

function caclulateLength(value) { if (value == null) { value = ''; } else { value = ngTrim && !isPasswordInput && angular.isString(value) ? value.trim() : value; } return String(value).length; }

I had a feeling that this was needed. Thanks for confirming it. Much cleaner 😃

gkalpak · 2018-04-25T17:54:05Z

src/components/input/input.spec.js

+ expect(pageScope.form.foo.$error['md-maxlength']).toBeFalsy();
+ });
+
+ it('should not trim spaces for required password inputs by default', function() {


Nit: This is not just "by default". It is the only posible behavior (i.e. it is not possible to override it).

gkalpak · 2018-04-25T17:55:51Z

src/components/input/input.spec.js

+
+ pageScope.$apply('foo = " "');
+ expect(pageScope.form.foo.$error['required']).toBeFalsy();
+ });


OOC, why not verify pageScope.form.foo.$error['md-maxlength'] too? (Possibly using foo = " " as well to make it truthy.)

no longer force ng-trim="false" trim the value by default or based on the ng-trim attribute don't trim password inputs to align with AngularJS behavior Fixes angular#10082

googlebot · 2018-05-04T21:21:21Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

josephperrott · 2018-05-04T21:21:46Z

Setting CLA as yes after confirming both listed PR authors have signed CLA.

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Feb 27, 2018

Splaktar changed the title ~~feat(mdMaxlength): support use with required and ng-trim~~ WIP: feat(mdMaxlength): support use with required and ng-trim Feb 27, 2018

Splaktar added this to the 1.1.9 milestone Feb 27, 2018

Splaktar self-assigned this Feb 27, 2018

Splaktar added the type: enhancement label Feb 27, 2018

samal-rasmussen mentioned this pull request Feb 28, 2018

mdMaxlength: Using mdMaxlength shows field with spaces as valid #1794

Closed

Splaktar force-pushed the mdMaxlengthNgTrim branch from 3b2e11c to 7dee818 Compare March 13, 2018 07:27

Splaktar modified the milestones: 1.1.9, 1.1.8 Mar 13, 2018

Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 13, 2018

Splaktar changed the title ~~WIP: feat(mdMaxlength): support use with required and ng-trim~~ feat(mdMaxlength): support use with required and ng-trim Mar 13, 2018

Splaktar added the needs: review This PR is waiting on review from the team label Mar 13, 2018

Splaktar requested review from jelbourn and devversion March 13, 2018 07:29

Splaktar force-pushed the mdMaxlengthNgTrim branch from 7dee818 to 95c45c0 Compare March 13, 2018 07:40

Splaktar modified the milestones: 1.1.8, 1.1.9 Mar 13, 2018

Splaktar force-pushed the mdMaxlengthNgTrim branch from 95c45c0 to 3b2a9cc Compare March 13, 2018 08:14

Splaktar removed the request for review from jelbourn March 13, 2018 08:52

Splaktar added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 13, 2018

jelbourn approved these changes Mar 13, 2018

View reviewed changes

Splaktar removed the needs: review This PR is waiting on review from the team label Mar 13, 2018

Splaktar force-pushed the mdMaxlengthNgTrim branch from 3b2a9cc to 2a48778 Compare April 19, 2018 23:35

googlebot added cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ labels Apr 19, 2018

Splaktar removed the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Apr 19, 2018

Splaktar added the needs: review This PR is waiting on review from the team label Apr 19, 2018

Splaktar modified the milestones: 1.1.9, 1.1.10 Apr 24, 2018

gkalpak approved these changes Apr 25, 2018

View reviewed changes

feat(mdMaxlength): support use with required and ng-trim="true"

147551d

no longer force ng-trim="false" trim the value by default or based on the ng-trim attribute don't trim password inputs to align with AngularJS behavior Fixes angular#10082

Splaktar force-pushed the mdMaxlengthNgTrim branch from 2a48778 to 147551d Compare April 26, 2018 03:08

googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Apr 26, 2018

Splaktar assigned andrewseguin and unassigned Splaktar Apr 26, 2018

Splaktar assigned josephperrott and unassigned andrewseguin May 4, 2018

Splaktar mentioned this pull request May 4, 2018

mdMaxlength: Always invalid for input type="number" #10216

Closed

josephperrott added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label May 4, 2018

josephperrott merged commit db1a85d into angular:master May 4, 2018

Splaktar deleted the mdMaxlengthNgTrim branch May 4, 2018 23:30

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

feat(mdMaxlength): support use with required and ng-trim (#11136)

046cccf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(mdMaxlength): support use with required and ng-trim #11136

feat(mdMaxlength): support use with required and ng-trim #11136

Splaktar commented Feb 27, 2018 •

edited

Loading

googlebot commented Feb 27, 2018

Splaktar commented Feb 27, 2018

Splaktar commented Feb 27, 2018

Splaktar commented Mar 13, 2018 •

edited

Loading

jelbourn left a comment

Splaktar commented Mar 13, 2018

gkalpak commented Mar 13, 2018

Splaktar commented Apr 19, 2018

gkalpak left a comment

gkalpak Apr 25, 2018

Splaktar Apr 26, 2018

gkalpak Apr 25, 2018

gkalpak Apr 25, 2018

googlebot commented May 4, 2018

josephperrott commented May 4, 2018

feat(mdMaxlength): support use with required and ng-trim #11136

feat(mdMaxlength): support use with required and ng-trim #11136

Conversation

Splaktar commented Feb 27, 2018 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

googlebot commented Feb 27, 2018

Splaktar commented Feb 27, 2018

Splaktar commented Feb 27, 2018

Splaktar commented Mar 13, 2018 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

Splaktar commented Mar 13, 2018

gkalpak commented Mar 13, 2018

Splaktar commented Apr 19, 2018

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Apr 25, 2018

Choose a reason for hiding this comment

Splaktar Apr 26, 2018

Choose a reason for hiding this comment

gkalpak Apr 25, 2018

Choose a reason for hiding this comment

gkalpak Apr 25, 2018

Choose a reason for hiding this comment

googlebot commented May 4, 2018

josephperrott commented May 4, 2018

Splaktar commented Feb 27, 2018 •

edited

Loading

Splaktar commented Mar 13, 2018 •

edited

Loading