Skip to content

Conversation

mayteio
Copy link

@mayteio mayteio commented Jul 31, 2019

Addresses #92.

Opening this early for discussion - I've only updated the TextField component. Just want to check I'm on the right track with your vision for the package.

One thing I wasn't sure of was the name prop. The new syntax doesn't pass down form, so we don't omit it from the MuiTextFieldProps type.

I'm also unsure on how to access isSubmitting - it's only passed to . Potentially useFormikContext() but then the component will re-render any time the form changes (not ideal).

Finally, not sure how you'd want to handle a missing name prop or existence of component prop - I've thrown a TypeError for now.

Any thoughts? Should I push on with the other components?

Cheers!

Harley Alexander added 2 commits July 31, 2019 20:00
… useField and switch from deprecated prop <Field component> to <Field as> syntax.
@mayteio mayteio changed the title Formik 2 / <Field as> prop #92 Formik 2 / <Field as> prop Jul 31, 2019
@cliedeman
Copy link
Collaborator

Interesting. I forgot the new hooks implementation would force a 2.0 upgrade. I will probably only release this once formik 2.0 reaches stable but can do a canary release at some point if we want to test the changes in projects

@mayteio
Copy link
Author

mayteio commented Jul 31, 2019 via email

package.json Outdated
{
"name": "formik-material-ui",
"version": "0.0.20",
"version": "1.0.20",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

versions bumps are automated

package.json Outdated
"peerDependencies": {
"@material-ui/core": ">=4.0.0",
"formik": ">=1.0.0",
"@material-ui/core": ">=4.3.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for updating the peer dep here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, just so that it was latest. I guess npm/semver will figure that out.


const fieldError = getIn(errors, name);
const showError = getIn(touched, name) && !!fieldError;
const [field, meta] = useField(name);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hook should be the in component below

export type TextFieldProps = FieldProps &
Omit<MuiTextFieldProps, 'error' | 'name' | 'onChange' | 'value'>;
FieldConfig &
Omit<MuiTextFieldProps, 'error' | 'onChange' | 'value'>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name should be omitted here

helperText: showError ? fieldError : props.helperText,
disabled: disabled != undefined ? disabled : isSubmitting,
helperText: showError ? error : props.helperText,
disabled: disabled !== undefined ? disabled : false,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: !== won't catch null

Is submitting is also needed for users who want to override the behaviour

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that simply disabled || false ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that works here

@9renpoto
Copy link
Contributor

mayteio#1 fixed conflict, and specs
please review it :), thank you.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.8%) to 78.947% when pulling 5d872da on mayteio:feature/formik-2 into 4fa842a on stackworx:master.

@cliedeman cliedeman force-pushed the master branch 2 times, most recently from ce76884 to b49624c Compare January 20, 2020 14:39
@cliedeman cliedeman closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants