Add a combination as-you-go and show-all-on-submit validator #10700
Conversation
| @@ -351,6 +361,8 @@ export function getFormValidator(form) { | |||
| return new AsYouGoValidator(form); | |||
| case CustomValidationTypes.ShowAllOnSubmit: | |||
| return new ShowAllOnSubmitValidator(form); | |||
| case CustomValidationTypes.AsYouGoShowAllOnSubmit: | |||
There was a problem hiding this comment.
How about we make custom-validation-reporting take multiple space-separated values, create a unified validator that takes the type of the validation(s) as a parameter and get rid of InteractAndSubmitValidator. I think the infrastructure is already there for this to happen and that way the syntax would be custom-validation-reporting = 'as-you-go show-all-on-submit'.
There was a problem hiding this comment.
some combinations may not make sense like show-first-on-submit show-all-on-submit but this would additionally allow show-first-on-submit as-you-go which is nice. @cvializ is this possible/easy to implement?
There was a problem hiding this comment.
I think that should be ok, it would just show all on submit.
|
It's definitely possible, but I think it would be a non-trivial amount of
work. We can make a separate issue for composable form validators, or if we
need to add more composed validators in the future to keep it from getting
clunky.
…On Fri, Jul 28, 2017 at 5:37 PM, Wassim Gharbi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-form/0.1/form-validators.js
<#10700 (comment)>:
> @@ -351,6 +361,8 @@ export function getFormValidator(form) {
return new AsYouGoValidator(form);
case CustomValidationTypes.ShowAllOnSubmit:
return new ShowAllOnSubmitValidator(form);
+ case CustomValidationTypes.AsYouGoShowAllOnSubmit:
I think that should be ok, it would just show all on submit.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10700 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ANd2kEbvb_Mm1Jhzblhz1JsXkez_Xt1pks5sSn6vgaJpZM4OnDTF>
.
--
You received this message because you are subscribed to the Google Groups
"amphtml-eng-github" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to ***@***.***
To view this discussion on the web visit https://groups.google.com/a/
google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/
pull/10700/review/53063964%40github.com
<https://groups.google.com/a/google.com/d/msgid/amphtml-eng-github/ampproject/amphtml/pull/10700/review/53063964%40github.com?utm_medium=email&utm_source=footer>
.
--
You received this message because you are subscribed to the Google Groups
"amphtml-eng" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to ***@***.***
To view this discussion on the web visit https://groups.google.com/a/
google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/
10700/review/53063964%40github.com
<https://groups.google.com/a/google.com/d/msgid/amphtml-eng/ampproject/amphtml/pull/10700/review/53063964%40github.com?utm_medium=email&utm_source=footer>
.
|
|
I think it's worth it, because once people start using it there's really no way to fix it without breaking pages... |
|
I don't think it would necessarily break the page, it would just be documented as a deprecated alias for |
|
I am going to merge and open a refactoring issue to track @wassgha's suggestion |
Closes #9427
I'm open to suggestions for changing the name of the validator. It works, but it's a little verbose.