Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
feat(forms): allow min/max validator to be bound to null #42978
Conversation
|
@AndrewKushnir I will be adding test cases as well once I get go ahead with current implementation. |
Thanks for creating this PR @iRealNirmal
I've left a comment with a proposal to move the enabled logic into the base class, please let me know what you think. Thank you.
iRealNirmal
force-pushed
the
addNull-in-min-max-validator
branch
3 times, most recently
from
e0c8e50
to
65783df
Compare
Thanks for additional changes @iRealNirmal, please see my comments below.
I have done this changes just one dilemma here in // TODO: add docs as needed. you want me to add comment right?(You don't need to provide comment I just want to confirm) or you meant it to be added some where else?
Yes, we need to add docs for that function (in a similar way other AbstractValidatorDirective class methods are documented).
iRealNirmal
force-pushed
the
addNull-in-min-max-validator
branch
from
65783df
to
19191ef
Compare
@iRealNirmal thanks for additional changes. I've left a few comments.
The general logic looks good, it'd be great to add tests to verify new behavior (based on the tests that you've added for MinLength and MaxLength validators in the previous PR).
Yes I will add test case as well, I was just waiting for our code approach to be finalised before writing test cases to prevent rewriting of it. I will add in next commit with suggested changes. |
iRealNirmal
force-pushed
the
addNull-in-min-max-validator
branch
2 times, most recently
from
3c5de3b
to
51f828a
Compare
|
@AndrewKushnir can you please review it again once ? |
|
@iRealNirmal sorry for the delay! I will have a look at the changes within the next day or so and let you know. Thank you. |
@iRealNirmal I've added some comments, could you please have a look when you get a chance? It also seems like there is a merge conflict with the main branch. It'd be helpful to rebase and resolve conflicts, so that we can run extra tests in Google's codebase.
iRealNirmal
force-pushed
the
addNull-in-min-max-validator
branch
2 times, most recently
from
d113588
to
2a6ce84
Compare
|
FYI I've also started tests in Google's codebase (internal-only link) to see if there are some use-cases that we'd need to cover. |
…ng the value to `null`) This commit updates the logic of the `min` and `max` validators to allow disabling them dynamically in case `null` is provided as a value. For example: `<input type="number" [min]="minValue">`, when `minValue` might be set to `null` in a component class. This should allow `min` and `max` validators to be used for dynamic forms. Note: similar support was added to the `minLength` and `maxLength` validators earlier (see angular#42565).
iRealNirmal
force-pushed
the
addNull-in-min-max-validator
branch
from
4c0e276
to
dc33248
Compare
|
@iRealNirmal just want to let you know that this PR was merged and this feature will be included into Angular v13 release! I also want to take this opportunity to say Thank You for the work that you did in this and other PRs! We made a few iterations on this change and you've addressed all the feedback and was very responsive. Thanks again for all your help! |
@AndrewKushnir thank you, thank you very much for your kind words and constant guidance, it was pleasure and fun working on it. Also am happy to give back community as am using angular since quite a long time and indirectly it helps to understand work being done by framework under the hood. As next item I will work on remaining validators, also feel free to let me know if there is anything else you want me to take care. |
|
@iRealNirmal thanks for the reply. If you get a chance to help, it'd be great to transition other validators (starting with minlength and maxlength) to use |
|
sure I will take care of it. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
If the validator is bound o be
ull then no validation occurs and attribute is not added to DOM.
PR already raised for minLength and maxLength valdiator (#42565).
Changes were discussed in #42378
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Null data type is not supported
Issue Number: #42267.
What is the new behavior?
Added support for null data type
Does this PR introduce a breaking change?
This is one of many PR which is going to be raised for adding support of null validation. One can go through conversation here
#42378 (comment)
Other information