-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add noValidate to Form by default #2402
Conversation
Build successful! 🎉 |
@@ -388,7 +390,7 @@ function FormWithSubmit() { | |||
} else { | |||
setFormStatus('progress'); | |||
} | |||
}, [policies, policiesDirty, pet, petDirty, truth, truthDirty, isSubmitted]); | |||
}, [policies, policiesDirty, pet, petDirty, truth, truthDirty, isSubmitted, emailDirty]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add email
to the dep array? And put them before isSubmitted
(nit)
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think it's fine that we don't provide an override since we are already leaning so heavily towards disregarding/disabling native browser validation already and don't have any components that have required
anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Any concerns over someone using their own custom inputs in Form that do still have 'required'?
I think since this is Spectrum-only it's probably ok. |
Related to #2401. If you use
type="email"
,pattern
, etc. native form validation UI shows rather than spectrum's help text. This addsnoValidate
to<Form>
by default to avoid this.Question: do we need a way to override this? I can't think of a valid use case that wouldn't be better served by custom Spectrum validation.