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

Improve handling of non event values in handleChange and handleBlur #1216

Merged
merged 9 commits into from Apr 2, 2019

Conversation

@MrLoh
Copy link
Contributor

MrLoh commented Dec 30, 2018

This PR improves handleChange and handleBlur for cases where they are not passed an event, for usage with React Native (Web) and other UI libraries, where the library currently struggles.

  • handleBlur can now be called with undefined instead of an event.
  • handleChange can now no only be called with a string value, but also any other value

The gist of this PR is checking for isEvent instead of checking for !isString. This makes the handling much more straight forward. I also took the liberty to refactor some of the code to be less dense (no confusing ternary assignment returns) and improve the types for handleBlur.

@MrLoh MrLoh changed the title Don't access e.target in handleBlur if not needed Improve handling of non event values in handleChange and handleBlur Dec 31, 2018
@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Dec 31, 2018

Circle CI only fails because credentials are missing. The tests all pass.

@krizka

This comment has been minimized.

Copy link

krizka commented Jan 3, 2019

@jaredpalmer Please approve this!

Copy link
Owner

jaredpalmer left a comment

Would it be faster or better to just check the execution environment?

Copy link
Owner

jaredpalmer left a comment

Some nits.

src/utils.ts Outdated Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
src/Formik.tsx Outdated Show resolved Hide resolved
@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Jan 7, 2019

@jaredpalmer thanks for reviewing this quickly! This PR addresses more than only issues with RN. Checking the value for isInputEvent instead of checking for isString allows usage of Formik with any ui library that doesn't return string values. Basically if the field returns an event, Formik will handle converting it to the right type. Otherwise the input component can give back any type (Number, Date, Array of Strings, ...) and Formik will take the value. A check of the execution environment wouldn't be able to help with that.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Jan 21, 2019

@jaredpalmer any more changes needed? This should be ready to merge.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Feb 6, 2019

Sorry for the delay, I was traveling, changed the type from any to unknown where relevant

@bsr203

This comment has been minimized.

Copy link

bsr203 commented Feb 13, 2019

hi I too like to have this functionality as I am not sure how to use a custom component otherwise. Mine is a Select component from an external library which provides the value at onChange callback. using string value is not a great option as the field value may be an id, but select component may display other attributes. so need to pass an object as value. Please let me know if it is possible now. thanks

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Feb 13, 2019

@bsr203 your usecase sounds like mine. You can install my fork branch from github and try it out.

@bsr203

This comment has been minimized.

Copy link

bsr203 commented Feb 13, 2019

thanks @MrLoh Meanwhile I found form.setFieldValue API allow any value to set to the field.

src/Formik.tsx Show resolved Hide resolved
@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Mar 4, 2019

@jaredpalmer can we please get this moving forward, it is starting to get a bit frustrating, I’ve made all the effort I can and after 2 months this has still not been merged 😞. I understand that time is limited for this open source project but it’s frustrating for contributors.

@MrLoh

This comment has been minimized.

Copy link
Contributor Author

MrLoh commented Apr 2, 2019

@Andreyco @jaredpalmer any updates on this?

Copy link
Owner

jaredpalmer left a comment

We should make sure this works with React Native

src/utils.ts Show resolved Hide resolved
Copy link
Owner

jaredpalmer left a comment

Fix TS error: unkown -> unknown

src/Formik.tsx Outdated Show resolved Hide resolved
jaredpalmer added 2 commits Apr 2, 2019
@jaredpalmer

This comment has been minimized.

Copy link
Owner

jaredpalmer commented Apr 2, 2019

I think this will work meow

@jaredpalmer jaredpalmer merged commit 899b9e9 into jaredpalmer:master Apr 2, 2019
5 checks passed
5 checks passed
WIP ready for review
Details
ci/circleci: deploy-docs Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
security/snyk - package.json (jaredpalmer) No manifest changes detected
security/snyk - website/package.json (jaredpalmer) No manifest changes detected
@jhoch

This comment has been minimized.

Copy link
Contributor

jhoch commented Apr 3, 2019

@MrLoh @jaredpalmer how was this intended to work with name or id? field is not set in the case of a non-event first-argument for handleChange. Wondering if this is intentional or an oversight.

I've just started looking at Formik and happen to want to use it with react-number-format which exposes onValueChange, which passes a value directly, instead of an event. I could see handleChange taking two arguments, (1) value and (2) name. Perhaps there's an appropriate "advanced" API in Formik already that I haven't found yet though!

Apologies, I mis-read the code. The following worked for me:

   onValueChange={({ value: changedValue }) => {
     onChange(name)(changedValue);
   }}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.