Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(forms): clarify that `minLength` and `maxLength` should be used w… #36242

Closed

Conversation

@sonukapoor
Copy link
Contributor

sonukapoor commented Mar 25, 2020

…ith strings/arrays only

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@googlebot googlebot added the cla: yes label Mar 25, 2020
@pullapprove pullapprove bot requested a review from AndrewKushnir Mar 25, 2020
@sonukapoor sonukapoor force-pushed the sonukapoor:min-max-length-update-docs-note branch from d3d0450 to a2ffca5 Mar 25, 2020
@sonukapoor sonukapoor force-pushed the sonukapoor:min-max-length-update-docs-note branch from a2ffca5 to f461c61 Mar 25, 2020
Copy link
Contributor

AndrewKushnir left a comment

Thanks for the PR @sonukapoor! I left a couple comments, please have a look when you get a chance.

Also I think that both commit messages should start with docs(forms):, since we are updating docs in both of them.

@@ -262,7 +262,8 @@ export class Validators {
* @description
* Validator that requires the length of the control's value to be greater than or equal
* to the provided minimum length. This validator is also provided by default if you use the
* the HTML5 `minlength` attribute.
* the HTML5 `minlength` attribute. Note that the `minLength` validator only works with

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Mar 26, 2020

Contributor

I'd propose to rephrase it a bit to mention the length property, for example:

Note that the `minLength` validator only works for types that have numeric `length` property, such as strings or arrays.

The `minLength` validator logic is also not invoked for values when their `length` property is 0 (for example in case of an empty string or an empty array), to support optional controls. You can use the standard `required` validator if empty values should not be considered valid.

This is just a proposal, please feel free to update is as you see fit.

@@ -100,6 +100,14 @@ Note that:
* This example adds a few getter methods. In a reactive form, you can always access any form control through the `get` method on its parent group, but sometimes it's useful to define getters as shorthands
for the template.

<div class="alert is-helpful">

This comment has been minimized.

Copy link
@AndrewKushnir

AndrewKushnir Mar 26, 2020

Contributor

I think we can keep the Built-in validators generic and move additional information from this section into individual validator descriptions. You already updated them, so may be we can just expand them a little bit?

This comment has been minimized.

Copy link
@sonukapoor

sonukapoor Mar 27, 2020

Author Contributor

Closed this PR, in favor of the previous one:

#36157

@sonukapoor sonukapoor closed this Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.