Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update to MUI5 #291

Merged
merged 28 commits into from
Oct 21, 2021
Merged

Update to MUI5 #291

merged 28 commits into from
Oct 21, 2021

Conversation

fxlemire
Copy link
Contributor

@fxlemire fxlemire commented Sep 26, 2021

This PR picks up where @smo043 left with his draft PR. I could unfortunately not target his PR with this one since it's in a draft state.

I've tried to scope my commits as best as I could to make the review easy. I had to take a design decision for the pickers as they changed quite a lot since V4. I'm unsure about the direction I took, so feel free to modify as you see fit.

I did not write the documentation on migration yet since the design direction could potentially change.

As part of the "rebranding" commit (d822918), I've only changed what referenced to the real MUI framework. I've kept formik-material-ui and all the material-ui references as they are, as I don't know if we want to rebrand the packages to formik-mui along with the documentation URLs.

With some hope, this will help with speeding up the release of MUI V5 support. Thanks for maintaining the repo @cliedeman!

This was referenced Sep 26, 2021
## Autocomplete
`TextField` props may be specified inside the `textField` prop. If no `renderInput` function is provided, the `textField` props are forwarded to the `TextField` input.

When using picker components initialize the starting value to `new Date()` and not the empty string
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a dev mode warning if we encounter a date with a string value

Copy link
Collaborator

@cliedeman cliedeman Oct 4, 2021

Choose a reason for hiding this comment

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

Is this stated somewhere in the mui5 docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not the author of this content, I've simply moved over the old material-ui-pickers documentation

https://github.com/stackworx/formik-material-ui/blob/master/docs/docs/api/material-ui-pickers.md?plain=1#L8

"react-dom": "^16.13.1"
"@docusaurus/core": "^2.0.0-beta.6",
"@docusaurus/preset-classic": "^2.0.0-beta.6",
"classnames": "^2.3.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: mui seems to have switched to clsx and I should switch this project over too at some point

@cliedeman
Copy link
Collaborator

@smo043 and @fxlemire thanks this is fantastic.

I have gone through the code and it looks good to go. I am going to run through the stories and then we can do some beta releases

@max-degterev
Copy link

Any chance to get this out as a beta?

@fxlemire
Copy link
Contributor Author

@suprMax To unblock myself I've temporarily published an alpha version of this PR under @mercantile/formik-material-ui and @mercantile/formik-material-ui-lab. Feel free to use this until this gets officially published.

💡 I recommend using an alias to avoid changing the imports later on: npm i formik-material-ui@npm:@mercantile/formik-material-ui@alpha

- Include FormControl, FormHelperText, InputLabel in `Select` component
- Fix stories and add new ones
- Update API doc
- Remove FAQ: Validation Errors Missing From Select
@cliedeman cliedeman merged commit d177c50 into stackworx:master Oct 21, 2021
@cliedeman
Copy link
Collaborator

@fxlemire @smo043

Thanks for the effort.
this has been released as

  • formik-mui-lab@1.0.0-alpha.3
  • formik-mui@4.0.0-alpha.3

@fxlemire fxlemire deleted the fxlemire/mui5 branch October 21, 2021 23:20
@fxlemire
Copy link
Contributor Author

fxlemire commented Oct 21, 2021

Pleasure @cliedeman ! FYI, there is still some polishing needed around the implementation (on top of better documentation, mostly for Select and the Pickers`). For example:

  • I'm not sure that validateOnChange and validateOnBlur are respected for all components that I've touched significantly (e.g. the pickers and Select)
  • Found out recently that the DatePicker starts validating ASAP as opposed to once the field gets marked as touched. Tried fixing it but in the time I've put, didn't manage 😕 Some help there would be great
  • There's insane code repetition in the picker implementation, we should do some refactoring there
  • After some thoughts I think I introduced a performance issue (not sure? wanted to test but lacking time these days) specifically here:
    ((params) => (
    <TextField
    {...params}
    error={showError}
    helperText={showError ? fieldError : helperText}
    label={label}
    onBlur={
    onBlur ??
    function () {
    setFieldTouched(field.name, true, true);
    }
    }
    {...textField}
    />
    We probably want to extract that out of the component, because I think this renders a new component every single time now 🤔
  • I think the picker props need some logic change to respect validateOnChange=false, validateOnBlur=true, something like what's below.
    // ...
    onChange:
      onChange ??
      function (date) {
        // Do not switch this order, otherwise you might cause a race condition
        // See https://github.com/formium/formik/issues/2083#issuecomment-884831583
        if (getIn(touched, field.name)) {
          setFieldTouched(field.name, true, false);
        }
        setFieldValue(field.name, date, validateOnChange ?? true);
      },
    onClose:
      onClose ??
      (() => {
        setFieldTouched(field.name, true, validateOnBlur ?? true);
      }),
    // ...

For lack of time, I don't think I'll be able to tackle these items over the next month.

That's about it. Cheers y'all, enjoy MUI!!

@cliedeman
Copy link
Collaborator

@fxlemire thanks for the insights, I will create tickets for the issues so we can tackle them

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants