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

feat(forms): introduce min and max validators #39063

Closed
wants to merge 3 commits into from

Conversation

sonukapoor
Copy link
Contributor

@sonukapoor sonukapoor commented Sep 30, 2020

This commit adds the missing min and max validators.

BREAKING CHANGE:

Previously min and max attributes defined on the <input type="number">
were ignored by Forms module. Now presence of these attributes would
trigger min/max validation logic (in case formControl, formControlName
or ngModel directives are also present on a given input) and
corresponding form control status would reflect that.

Fixes #16352

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:

Does this PR introduce a breaking change?

  • Yes
  • No

@sonukapoor sonukapoor force-pushed the min_max_validators branch 2 times, most recently from fe44e5e to 6fea537 Compare October 1, 2020 01:54
@ngbot ngbot bot added this to the needsTriage milestone Oct 1, 2020
@AndrewKushnir AndrewKushnir added target: major This PR is targeted for the next major release action: presubmit The PR is in need of a google3 presubmit labels Oct 1, 2020
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Oct 1, 2020

Initial presubmit to see if it breaks targets in g3 (+ global presubmit as well).

@sonukapoor sonukapoor force-pushed the min_max_validators branch 2 times, most recently from 3492716 to 971cf8d Compare October 1, 2020 12:30
@sonukapoor sonukapoor marked this pull request as ready for review October 1, 2020 12:31
@sonukapoor sonukapoor force-pushed the min_max_validators branch 3 times, most recently from c16413b to 354c930 Compare October 2, 2020 13:37
@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Nov 4, 2020
@sonukapoor sonukapoor force-pushed the min_max_validators branch 8 times, most recently from 6fea895 to c9fde70 Compare December 30, 2020 18:34
@sonukapoor sonukapoor force-pushed the min_max_validators branch 2 times, most recently from b8ddaff to 9540729 Compare December 31, 2020 16:55
@sonukapoor
Copy link
Contributor Author

@AndrewKushnir Thanks for the updated information. The changes and the refactor of the unit tests look good to me.

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Feb 4, 2021
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

sonukapoor and others added 3 commits February 4, 2021 16:47
This commit adds the missing `min` and `max` validators.

BREAKING CHANGE:

Previously `min` and `max` attributes defined on the `<input type="number">`
were ignored by Forms module. Now presence of these attributes would
trigger min/max validation logic (in case `formControl`, `formControlName`
or `ngModel` directives are also present on a given input) and
corresponding form control status would reflect that.

Fixes angular#16352
@mary-poppins
Copy link

You can preview 76d699b at https://pr39063-76d699b.ngbuilds.io/.

@AndrewKushnir
Copy link
Contributor

Thanks for the review @atscott, great catch on string vs string|number type! 👍

I've pushed a fixup commit to adjust types. Could you please have another look when you get a chance?

@AndrewKushnir AndrewKushnir requested review from atscott and removed request for alxhub February 5, 2021 00:58
@AndrewKushnir
Copy link
Contributor

FYI, also started a new Global Presubmit.

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Feb 5, 2021
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the request for review from jelbourn February 5, 2021 18:39
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Feb 5, 2021
@AndrewKushnir
Copy link
Contributor

The feedback was addressed and the most recent TGP run is successful, so I'm adding this PR to the merge queue.

@sonukapoor huge thanks for working on this feature and addressing all the comments 👍
Also thanks to the reviewers for provided feedback!

@AndrewKushnir
Copy link
Contributor

Note to Caretaker: this PR is ready for merge, the TGP is "green" (only one unrelated build failure). It'd still be great to sync it in g3 as a separate/individual change to rollback easier if needed. Thank you.

@sonukapoor
Copy link
Contributor Author

Always a pleasure @AndrewKushnir

@Airblader
Copy link
Contributor

Airblader commented Mar 13, 2021

Does this affect applications which specify both the attribute and separately a corresponding validator in any way? Or would the validator then just run twice without noticable effects?

@AndrewKushnir
Copy link
Contributor

@Airblader this change adds new min/max validators that would be invoked on <input type="number"> (when min/max attributes are present). This change does not block other existing (custom) validators from being executed.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker aio: preview area: forms breaking changes cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular's input validation should align with HTML5 validation! (Currently it does not!)
8 participants