-
-
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
Add parse, format, formatOnBlur to getFieldProps() #2255
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/formium/formik/4ep2977gn |
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 ca194f7:
|
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.
The functionality is looking pretty good here, but I think we can go a step further by using the new parse and format props to handle the special cases of checkboxes and radios, decoupling that functionality from handleChange. Then we wouldn't have to use a different onChange
for checkboxes vs normal fields.
Also need to use unknown
for field value on parse.
packages/formik/src/Formik.tsx
Outdated
const { type, value, checked, options, multiple } = target; | ||
let val; | ||
let parsed; | ||
val = /number|range/.test(type) |
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 this entire callback should be the default parse
function. If I pass a callback to parse, I'd want to receive the raw value of the input field and not the parsed value of the input field, so it would override this function entirely. I think we might want to make this function available to the fieldProps as well, if it uses anything not available from outside of this scope. like field.defaultParseFn or something.
That way a user could do something like parse={(value, field) => field.defaultParseFn(parseInt(value, 10), field)}
If it doesn't use anything within this scope, this should just be a constant function exported from this file. so we could do something like
import { defaultParseFn } from 'formik';
() => <Field parse={(value, field) => defaultParseFn(parseInt(value, 10), field)} />
This would let us fix issues like #2044 with custom parse and format props.
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've revised my thoughts, and think onChange should handle the grouped change, so parse fn should receive a single value, but that value should be unparsed, meaning no parseFloat(value) here.
Then onChange can be overridden by the parent component.
// In addition, to support `parse` fn, we can't just re-use the OG `handleChange`, but | ||
// instead re-implement it's guts. | ||
if (type !== 'radio' && type !== 'checkbox') { | ||
field.onChange = (eventOrValue: React.ChangeEvent<any> | any) => { |
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 the parse function provides the perfect opportunity to decouple the checkbox and radio parsing from handleChange, converting value: unknown
to a custom shape. Then if a user wants to override this logic, they can easily.
// imagine I usedCallback() and stuff
const handleChange = (value: any, field: FieldProps<any>) =>
setFieldValue(field.name, field.parse(field.value, field);
// note this function explicitly allows `any` value, not necessary to use unknown
const defaultParseFn = (value: any, field: FieldProps<any>) => {
/number|range/.test(field.type)
? ((parsed = parseFloat(value)), isNaN(parsed) ? '' : parsed)
// checkboxes
: /checkbox/.test(type)
? getValueForCheckbox(getIn(state.values, fieldName!), checked, value)
// <select multiple>
: !!multiple
? getSelectedValues(options)
: value;
}
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.
Problem is that checkbox relies on current state for getValuesForCheckbox(). Technically we would need to provide that. What do you think?
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.
If that's the case, I think we can provide field.defaultParseFn
or something.
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.
Oh, you mean in the event the user wants to getValuesForCheckbox
in userland? hmm.. maybe we'll need to pass formikBag as a third argument, or expose the field's complete value in some way.
If you can figure out a way to use parse
from outside of Formik (with as little Formik surface area as possible...) to update a multi checkbox field, I think that's something that would be required of the parse function. I use that in my onParse function for a custom CheckboxGroup that I use.
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've revised my thoughts here, and I agree that onChange should handle the array parsing for checkboxes and multi-selects. But getValueOrEvent should return an unparsed single value, with the default parse function for a number or range field running parseFloat for backwards compatibility.
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.
TLDR: keep values as strings for the default.
FWIW currently, in the app, I'm working on I have to both serialize and parse numbers to make sure they're expected values when Formik is transforming them correctly and GraphQL knows its an Int.
I get initialValues
from query parameters in the URL, which are all strings. Then I parse them into Numbers if they're defined, and if they're not, I need to cast them to ''
. Next, I need to make sure they're numbers, to then tell GraphQL this is an int or fallback to null
to not use the variable. This would have been a lot easier had Formik not changed the values to a number. because then I could just cast them when making the query to GraphQL.
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.
@lifeiscontent you can override this functionality yourself with parse={value => value}
we need to keep the default parsing ints for backwards compatibility for now.
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've changed my mind on the array parsing parts, I think they make sense in onChange
. However, we want to make sure getValueFromEvent
returns an unparsed value (not number for number/range inputs) in case the user wants to write a custom number parse function.
packages/formik/src/Formik.tsx
Outdated
const { type, value, checked, options, multiple } = target; | ||
let val; | ||
let parsed; | ||
val = /number|range/.test(type) |
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've revised my thoughts, and think onChange should handle the grouped change, so parse fn should receive a single value, but that value should be unparsed, meaning no parseFloat(value) here.
Then onChange can be overridden by the parent component.
// In addition, to support `parse` fn, we can't just re-use the OG `handleChange`, but | ||
// instead re-implement it's guts. | ||
if (type !== 'radio' && type !== 'checkbox') { | ||
field.onChange = (eventOrValue: React.ChangeEvent<any> | any) => { |
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've revised my thoughts here, and I agree that onChange should handle the array parsing for checkboxes and multi-selects. But getValueOrEvent should return an unparsed single value, with the default parse function for a number or range field running parseFloat for backwards compatibility.
@johnrom can you make your suggested changes and PR to this branch? |
@jaredpalmer I'm mid-project at the moment but if I get some time this week/end I'll open a PR |
* Moved default number parse logic from onChange to parse * tests: onChange is no longer equal to handleChange * Remove parse and format from input props
Any updates on this one? |
@jaredpalmer @johnrom any update on this? |
My PR is complete and merged so it's just a matter of maybe some more testing and releasing when maintainers have availability. |
@yekver this project is maintained by volunteers, so for a lower priority change like this you may be better off implementing yourself by wrapping edit: I think this is a better example #2411 (comment) |
This feature has high priority. It's a bit strange that such an important PR isn't merged for a half a year |
Hi there @jaredpalmer @johnrom - any update on this? |
@yekver I agree, but projects maintained by volunteers often have periods of rapid change and lulls in between. @YuriBarssi I have contributed all I can to this. |
Size Change: +1.6 kB (3%) Total Size: 42.4 kB
ℹ️ View Unchanged
|
🦋 Changeset detectedLatest commit: 18f82f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
righteous! |
@johnrom @jaredpalmer please give an example of the formatString used in the example above |
This is part of v3 release and thus the documentation for it will likely be created when or shortly before it is released. |
Ok, it worked here, thanks |
@johnrom any idea on when 3.0 is being released? |
Hi, not sure where to ask/mention this, but was trying out the phone number example on the latest alpha release. One issue IMO is that when you move your cursor back to edit something in the middle, the cursor jumps back to the end of the input, which is pretty frustrating to users. any thoughts on that? I believe some other things i've been exploring do some things on key down, and I don't seem to see the same issue. My use case is setting an initial value in cents, then formatting it into dollars on the way in, and on the way out turn it back into cents. also, any thoughts on storing numbers in formik state, and doing this sort of conversion to string and then back to number? |
@kelly-tock masking is definitely one of the harder things to accomplish and will have to be some sort of dressing on top of the base parse/format functionality, or even handled in userland with a special component passed to Field which handles the cursor. Masking was discussed here, but didn't make it into this PR. #1525 In terms of storing numbers in Formik state, it's a bit of a difficult one. Ultimately, I've come to the conclusion that for v4 this is the flow a field should take, after v3 is merged with this base parse and format functionality: Formik.values = { myNumber: 1 }
Field.value = "1"
Field.onChange("1.")
Formik.values = { myNumber: NaN } // parse returns NaN when field is not a valid number
Field.value = "1." // format does not overwrite the previous string when the value is NaN
Field.onChange("1.0")
Formik.values = { myNumber: 1.0 }
Field.value = "1.0" However this changes the way that format works, so it may not release until the next major version. However, if we can do it in a backwards compatible way, we might be able to land it in v3.x |
Improvements
Adds
parse
,format
, andformatOnBlur
to field options.Breaking Changes
onChange
andonBlur
functions returned bygetFieldProps()
(and thususeField
/<Field>
) are no longer equivalent toformikProps.handleChange
andformikProps.handleBlur
. Instead, they are specifically scoped to the relevant field (AND thus cannot be curried anymore). However, they can both either accept an event or value as arguments. This means we no longer necessarily need thehelpers.setValue
andhelpers.setTouched
. This should make it much easier for React Native folks and less awkward for everyone.Begin deprecation of
formikProps.handleChange
andformikProps.handleBlur
After this change, there is no reason to use aside for backwards compatibility either
formikProps.handleChange
orformikProps.handleBlur
. They are both inferior to theonChange
andonBlur
functions returned bygetFieldProps()
because they lack the ability to utilizeparse
andformat
.Example
Close #1525 #1876
View rendered examples/parse-format/README.md