-
-
Notifications
You must be signed in to change notification settings - Fork 343
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
fix(core): remove mount validation on touch #726
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f25c26b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
This really tripped our team up. We expected that if we specify onMount + onChange validators then when the field is "changed" it would clear the onMount error. I think clearing the mount error on change / blur is probably a reasonable way to address it. |
facf85e
to
9be53d4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #726 +/- ##
==========================================
- Coverage 91.55% 88.50% -3.05%
==========================================
Files 21 26 +5
Lines 900 1079 +179
Branches 206 267 +61
==========================================
+ Hits 824 955 +131
- Misses 71 115 +44
- Partials 5 9 +4 ☔ View full report in Codecov by Sentry. |
This PR closes #689
A few considerations:
When should we remove the errors?
My proposal would be to remove each field individually when gets touched and the form as soon as a field is touched.
As long as a field is not touched, it keeps the mount errors.
Should we be able to submit a form with onMount errors?
I'd say no, but currently if
state.submissionAttempts === 0 && !isTouched
we havecanSubmit: true
Adding
&& !hasOnMountError
also aims to fix the unwanted behavior described incanSubmit
is alwaystrue
on first render of form #723We might want to explicitly explain in the docs how we intend onMount to behave
I'm happy to improve the docs in this PR if we decide to go through with the proposed behavior
Bonus
The amount of eslint disable on form.errorsMap makes me wonder if we should consider it as optional in first place, but that's maybe another topic.