-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Run validationSchema for validateField calls #1784
Conversation
I would love to see this merged 🙌🏻 |
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.
Some thoughts on design patterns.
src/Formik.tsx
Outdated
@@ -423,7 +423,19 @@ export function useFormik<Values extends FormikValues = FormikValues>({ | |||
return Promise.resolve(maybePromise as string | undefined); | |||
} | |||
} else { |
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.
I think it'd be more readable to continue with positive checks and to return the resolved promise last.
if (isFunction(props.validate)) {
// ...
} else if (props.validationSchema) { /* validation code*/ }
return Promise.resolve();
Also, if in combination with my next review comment about validating a single field, we can have their callbacks match, we can combine these into a single if (maybePromise) { /* validate */ }
Thanks for the comments @johnrom - I'll try to get to these this weekend or next week! |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9417de9:
|
@johnrom are you good with this? |
@johnrom @jaredpalmer when can we expect this PR to be merged? |
In terms of validating a single field, I realized that a change might actually invalidate a different field, so my review can be disregarded. I don't use validation schema in my projects, so if you guys are good with it and have tested, feel free to move forward without my signoff! I might recommend adding a test though, to prevent future regressions. |
@johnrom I can fix the conflicts, add some tests, and also make some of the changes you suggested (such as “I think it’d be more readable to continue with positive checks and to return the resolved promise last...“. which is still relevant). Thanks for following up! |
Okay, update and I will merge |
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/jared/formik-docs/lntbbjn1r |
@jaredpalmer updated it with some tests & rearranged some logic |
Cool. LGTM |
Previously
validateField
would only work with avalidate={Function}
property on a field. Now you can performvalidateField('myFieldName')
and it will look at the providedvalidationSchema
to determine any errors.Fixes issue #1755