Skip to content
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 support for shouldValidate in FieldHelperProps of useField #2223

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mrmuhammadali
Copy link
Contributor

@mrmuhammadali mrmuhammadali commented Jan 17, 2020

I have a use case where I've disabled validateOnChange but wanted to run the validations using only setValue in case of a Checkbox as it only calls onChange. So I've added the support for shouldValidate in FieldHelperProps, i.e. setValue and setTouched.

Fixed Issue #2219 in this PR.


View rendered docs/api/useField.md

@vercel
Copy link

vercel bot commented Jan 17, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/jared/formik-docs/2sxfrnvkp
✅ Preview: https://formik-docs-git-fork-mrmuhammadali-master.jared.now.sh

@codesandbox-ci
Copy link

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 6010801:

Sandbox Source
Formik TypeScript Playground Configuration

@mrmuhammadali
Copy link
Contributor Author

@jaredpalmer / @johnrom
Can you guys look into this PR.

@johnrom
Copy link
Collaborator

johnrom commented Jan 20, 2020

@mrmuhammadali I appreciate your contribution, but pinging is not a good way to prioritize PRs.

@johnrom
Copy link
Collaborator

johnrom commented Jan 20, 2020

The code looks good to me and can be approved for implementing this specific technique, but I can't help feel this isn't the right place to decide whether to validate or not. I'd prefer we exposed a method that never validated so that someone could decide on their own to justSetFieldValue().then(shouldValidate ? validateField : Promise.resolve).

However, that's an API decision that I'll defer to someone else.

@mrmuhammadali
Copy link
Contributor Author

Sorry about that, I’m new to open source contribution but I’ll note that down.
Anyway, I think if setFieldValue and setFieldTouched have a third parameter of shouldValidate then setValue and setTouched should support that too, as internally these functions are using above mentioned methods.

@mrmuhammadali
Copy link
Contributor Author

In case of Dropdowns, Radio Buttons, Toggles and Checkboxes, we’ve to take the decision whether to validate the field or not, so that’s another argument to discuss. As in above mentioned cases, we’ve to call setTouched and setValue at the same time, i.e onChange.

@johnrom
Copy link
Collaborator

johnrom commented Jan 21, 2020

No worries, the contribution is great, I'm just wondering if Jared had purposefully left out the shouldValidate so that the decision to validate can be made somewhere else, and if so, where that might be and if it needs documenting.

@unframework
Copy link

I am the original reporter for #2219, thanks for making this PR @mrmuhammadali!

I can see how there might be a broader reason to not include shouldValidate flag, but this would be quite useful in some complex field use cases (came up twice for me now in the last couple months). As discussed above, for checkbox groups and toggles the conventional validate-on-blur/validate-on-change flags don't really apply anyway, so this flag feels like a fairly uncontroversial feature. Hope to see a decision on this soon, even though I understand that the team is busy.

Thanks folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants