timdeschryver / ng-signal-forms

120 stars 9 forks source link

feat: add `email` validator #39

Open michael-small opened 6 months ago

michael-small commented 6 months ago

Work in progress, would this be considered for acceptance

Reasoning

Outstanding work

michael-small commented 6 months ago

Great catch, I think we should try to keep as close to possible to the Angular behavior. Feel free to update the validator to also accept empty strings.

I could do that. However, I think if this is done, then it should be done for a few other validators as well. As per that file I link from their implementation, the same "don't validate empty values to allow optional controls" is done for:

Angular's validators names

maxLengthValidator doesn't have this but it's also kind of its own thing

This library has all of these as well and it looks like the equivalents also could use updating


Should they all be updated?

timdeschryver commented 6 months ago

Great catch, I think we should try to keep as close to possible to the Angular behavior. Feel free to update the validator to also accept empty strings.

I could do that. However, I think if this is done, then it should be done for a few other validators as well. As per that file I link from their implementation, the same "don't validate empty values to allow optional controls" is done for:

Angular's validators names

  • minValidator
  • maxValidator
  • emailValidator
  • minLengthValidator
  • patternValidator

maxLengthValidator doesn't have this but it's also kind of its own thing

This library has all of these as well and it looks like the equivalents also could use updating

Should they all be updated?

The ones that accept a string as value should be updated imho. This means min and max not because they receive a number and that's already being handler by checking for falsy values. Feel free to correct me if I'm missing something.

maxLength validator is missing from the list. This can also be done in a separate PR.