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

Upgrade to Material-UI 5 #1772

Merged
merged 14 commits into from Oct 25, 2021
Merged

Upgrade to Material-UI 5 #1772

merged 14 commits into from Oct 25, 2021

Conversation

zxhmike
Copy link
Contributor

@zxhmike zxhmike commented Sep 1, 2021

Make mui-datatables to be compatible with Material-UI 5. The target version is 5.0.1. The version of the mui-datatables itself is not bumped yet.

Followed migration instructions on this page: https://next.material-ui.com/guides/migration-v4/
Also used the codemod here: https://github.com/mui-org/material-ui/blob/next/packages/material-ui-codemod/README.md#mui-replace

Changes:

  • Use createTheme and ThemeProvider instead of createMuiTheme, MuiThemeProvider.
  • Use ThemeProvider instead of MuiThemeProvider.
  • Import withStyles, makeStyles from @mui/styles instead of @material-ui/core/styles.
  • Added ThemeProvider to the root of the examples page.
  • Upgrade material-ui/core, icons versions to 5.0.0-rc.0. Add @material-ui/styles version 5.0.0-rc.0.
  • Use @wojtekmaj/enzyme-adapter-react-17 instead of enzyme-adapter-react-16. Caution: This is an unofficial package. Cannot find an official enzyme adapter for react 17.
  • Upgrade next version to 11.1.2.
  • Upgrade react and react-dom versions to 17.0.2.
  • Upgrade react-waypoint version to 10.1.0.
  • Upgrade rollup version to 2.3.4. Since rollup-plugin-uglify is not compatible with rollup version 2, use @lopatnov/rollup-plugin-uglify instead. Caution: This is an unofficial package. Cannot find an official rollup uglify plugin in version 2. Also add rollup-pluginutils as the dependency of the new rollup uglify plugin.
  • Add @emotion/react and @emotion/styled as required by Material-UI 5.
  • Use react-sortable-tree-patch-react-17 instead of react-sortable-tree. Caution: This is an unofficial package.

Known Issues:

Able to bring up https://localhost:5050 after running npm run dev and run most examples except the following examples:

  • Column Filters: Showing "Sorry, no matching records found". Upon further review, this seems expected. Remove a filter to show data. Please confirm.
  • Column Option Update: Showing "Sorry, no matching records found". Upon further review, this seems expected. Remove a filter to show data. Please confirm.
  • Component: Does not render. Fixed by adding empty labels to FormControlLabel components. Reference: https://next.material-ui.com/guides/migration-v4/#formcontrollabel

We are looking into the above issues. Please let us know what else need to be tested. Thank you.

@HaimBendanan
Copy link

How are you progressing with this PR? Thanks for handling that 👍!

@zxhmike
Copy link
Contributor Author

zxhmike commented Sep 15, 2021

This PR is actually ready for review and merge. Please let me know what the next step is. Feedbacks welcome. Thanks.

@luluhoc
Copy link
Contributor

luluhoc commented Sep 17, 2021

I hope this will get merged soon as I want to move my project to v5.

@jasonlew
Copy link

Also eagerly awaiting this 😁

@luluhoc luluhoc mentioned this pull request Sep 24, 2021
@luluhoc
Copy link
Contributor

luluhoc commented Sep 28, 2021

@zxhmike I build your pr myself and put in my project but when I go to the route with mui datatables it removes almost all of my styles from my app.

@zxhmike
Copy link
Contributor Author

zxhmike commented Sep 28, 2021

@luluhoc Thanks for the update. Do you see any errors/warnings in the console? We compiled our own project using this branch and it is fine so far.

@AlonMiz
Copy link

AlonMiz commented Sep 28, 2021

Also eagerly awaiting this 😁

+1

@luluhoc
Copy link
Contributor

luluhoc commented Sep 28, 2021

@luluhoc Thanks for the update. Do you see any errors/warnings in the console? We compiled our own project using this branch and it is fine so far.

It was my fault, but now I can't do table pagination I'm getting, or change rows per page. Even in the examples page it is not working for me.

Uncaught TypeError: onPageChange is not a function
    at handleNextButtonClick (TablePaginationActions.js?4da9:46)
    at HTMLUnknownElement.callCallback (react-dom.development.js?61bb:3945)
    at Object.invokeGuardedCallbackDev (react-dom.development.js?61bb:3994)
    at invokeGuardedCallback (react-dom.development.js?61bb:4056)
    at invokeGuardedCallbackAndCatchFirstError (react-dom.development.js?61bb:4070)
    at executeDispatch (react-dom.development.js?61bb:8243)
    at processDispatchQueueItemsInOrder (react-dom.development.js?61bb:8275)
    at processDispatchQueue (react-dom.development.js?61bb:8288)
    at dispatchEventsForPlugins (react-dom.development.js?61bb:8299)
    at eval (react-dom.development.js?61bb:8508)

@luluhoc
Copy link
Contributor

luluhoc commented Sep 28, 2021

@zxhmike I submitted a PR to your branch, please review. It is working for me now.

pisrcio#1

@zxhmike
Copy link
Contributor Author

zxhmike commented Sep 29, 2021

Hi @luluhoc ,

Thanks for this PR. I saw that you changed something in the TablePagination.js file. However, if you look at our PR to mui-datatables (#1772), we did not make any change in those two lines (see the screenshot below), so this does not seem to be a bug we introduced while upgrading to mui 5. If you feel like it is a different bug, could you please create a new PR to mui-datatables instead? Thank you.
Screen Shot 2021-09-28 at 6 11 28 PM

@luluhoc
Copy link
Contributor

luluhoc commented Sep 29, 2021

Hi @luluhoc ,

Thanks for this PR. I saw that you changed something in the TablePagination.js file. However, if you look at our PR to mui-datatables (#1772), we did not make any change in those two lines (see the screenshot below), so this does not seem to be a bug we introduced while upgrading to mui 5. If you feel like it is a different bug, could you please create a new PR to mui-datatables instead? Thank you. Screen Shot 2021-09-28 at 6 11 28 PM

Naming of this two props was changed in this commit mui/material-ui@ab33be8#diff-a88e6d0575e76796d2fdea671ed8b3620762fbd4e4cdcfc01677fc4f3be6fadb
I think you should consider accepting this PR.

image

Co-authored-by: luluhoc <lucjan.grzesik@gmail.com>
@zxhmike
Copy link
Contributor Author

zxhmike commented Oct 1, 2021

Hi @luluhoc ,

I see. MUI changed its API when upgrading to version 5. I have already merged into this branch. Thanks again for the contribution.

Besides these two changes, are you aware of any other API changes of MUI that we should aware of when upgrading mui-datatables to support MUI v5? Or if you know such a changelist exist, please point us to. We would like to do a scan of mui-datatables code base to see if anything else if affected. Thanks.

Mike

@aidiss
Copy link

aidiss commented Oct 4, 2021

Thank you for wonderful work

@JonJoyce
Copy link

JonJoyce commented Oct 6, 2021

Is there any ETA for getting this merged? Working on MUI5 updates now and this is the only blocker.

@luluhoc
Copy link
Contributor

luluhoc commented Oct 7, 2021

Hi @luluhoc ,

I see. MUI changed its API when upgrading to version 5. I have already merged into this branch. Thanks again for the contribution.

Besides these two changes, are you aware of any other API changes of MUI that we should aware of when upgrading mui-datatables to support MUI v5? Or if you know such a changelist exist, please point us to. We would like to do a scan of mui-datatables code base to see if anything else if affected. Thanks.

Mike

Yes, I have one more problem I will try to do a PR later.

Overall, I've been running this PR in production, and I haven't encountered any problems whatsoever.

@zxhmike

this is the error I'm getting

Warning: React does not recognize the `buttonRef` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `buttonref` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

@d-knafo
Copy link

d-knafo commented Oct 11, 2021

+1

@yaniv-upstream
Copy link

@gregnb help us her. please

@kvlknctk
Copy link

This PR required and important. We can't use it like this.

@ashfaqnisar
Copy link
Contributor

Fixed most of the visible issues with this pisrcio#2. If anyone connects it to their project and finds any other issues, would be happy to help!

@shaun-scale
Copy link

I am new-ish to React, but just tried to pull this specific branch in because I was noticing what I imagine were CSS conflicts between MUI 5 and MUI 4 when muitable was being rendered.

Anyways, I now get the following exception on page load:
image

My table is simply like

const tableOptions = {
  filterType: "textField",
  print: false,
  selectableRows: "none",
  sortOrder: { name: "Description", direction: "asc" },
};

const columns = [
  "Name",
  "Description",
  {
    name: "Details",
    options: {
      filter: true,
      sort: false,
      download: false,
    },
  },
];

<MUIDataTable
  title={"Products"}
  data={productData}
  columns={columns}
  options={tableOptions}
/>

Could just be user error, but wanted to share if helpful

@ashfaqnisar
Copy link
Contributor

Hey @shaun-scale, I tried to reproduce the error but was not unable to do that. Would be able to share a code sandbox ?
While reproducing this error, found out that we need to increase the theme breakpoint down by one step as provided here in the migration guide.

@d-knafo
Copy link

d-knafo commented Oct 15, 2021

@gregnb please merge

@ashfaqnisar
Copy link
Contributor

ashfaqnisar commented Oct 15, 2021

Hey @gregnb, It would be great, if you merge the PR! 😀

@shaun-scale
Copy link

Let's chalk it up to user error then - but glad we found some other issues to address :)

@shaun-scale
Copy link

It may be helpful to google "LinkedIn Gregnb" - maybe his Github email isn't used by him anymore :)

@HeavenlyEntity
Copy link

HeavenlyEntity commented Oct 15, 2021

Issue: #1791 is currently breaking the production release of the library please push an update. 🙆‍♂️

pretty sure 50% of all issues will disappear after this merge. Waiting....lol

Grant a trusted contributor write access to merge, please 😫

@igor-pavlichenko
Copy link

igor-pavlichenko commented Oct 18, 2021

Yes, I have one more problem I will try to do a PR later.

Overall, I've been running this PR in production, and I haven't encountered any problems whatsoever.

@zxhmike

this is the error I'm getting

Warning: React does not recognize the `buttonRef` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `buttonref` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

@luluhoc
buttonRef has been renamed to just ref https://github.com/mui-org/material-ui/releases/tag/v5.0.0-alpha.32

@HeavenlyEntity
Copy link

HeavenlyEntity commented Oct 18, 2021

Yes, I have one more problem I will try to do a PR later.
Overall, I've been running this PR in production, and I haven't encountered any problems whatsoever.
@zxhmike
this is the error I'm getting

Warning: React does not recognize the `buttonRef` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `buttonref` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

@luluhoc buttonRef has been renamed to just ref https://github.com/mui-org/material-ui/releases/tag/v5.0.0-alpha.32

If this is added to PropTypes you should be fine. But this "error" is not such an error but a warning. You can still proceed to use that functional prop as intended. This warning is common amongst lots of UI libraries. It can be fixed only by the contributors if you're mui component is on a page and not a separate component file and exported. It will show in the console as it's not recognized by React after compilation so it passes through to the native browser. When the page mounts bable is the one that will ultimately recognize the prop on the component to not throw that warning. You can disregard it or if you're making this your own custom component in a file be sure to add it to our PropTypes so it disappears.

@gnowland
Copy link

gnowland commented Oct 18, 2021

@HeavenlyEntity while technically correct, the underlying issue would also be fixed once #1748 is merged, renaming the now-non-existent buttonRef prop to ref on the IconButton component.

FYI regarding the comments about finding gregnb on linkedin - it looks like @gabrielliwerant took over as maintainer somewhere around Jan 2019, then @patorjk became the maintainer on May 30, 2020 and subsequently handed maintainer responsibility to @wdh2100 on Mar 10, 2021.

@HeavenlyEntity
Copy link

HeavenlyEntity commented Oct 19, 2021

@HeavenlyEntity while technically correct, the underlying issue would also be fixed once #1748 is merged, renaming the now-non-existent buttonRef prop to ref on the IconButton component.

FYI regarding the comments about finding gregnb on linkedin - it looks like @gabrielliwerant took over as maintainer somewhere around Jan 2019, then @patorjk became the maintainer on May 30, 2020 and subsequently handed maintainer responsibility to @wdh2100 on Mar 10, 2021.

Yeah makes sense just needs to be updated. @wdh2100 , @gabrielliwerant , or @patorjk could you merge this PR as it is ready? The next PR to merge is the #1748 after this one to remove props/PropTypes. All new installs are breaking from the new mui update. Please and thank you 👍 😁

@mikejlachlan
Copy link

Really itching for this one 🤞

@luciano-fiandesio
Copy link

Any reason this PR is getting ignored by the mantainers?

@garciabruno
Copy link

Would be really nice to have this.

@wdh2100
Copy link
Collaborator

wdh2100 commented Oct 25, 2021

Release 4.0.0

@zxhmike, @gonsaje, @luluhoc, @ashfaqnisar

@ashfaqnisar
Copy link
Contributor

Release 4.0.0

@zxhmike, @gonsaje, @luluhoc, @ashfaqnisar

Thanks for merging this @wdh2100, you're the GOAT!

@Cirelion
Copy link

@wdh2100 After updating to 4.0.0 it breaks for me when using SSR, could this be due to the release? If so, I'll create a issue for it.

@ianpavlovski
Copy link

who knows when will be updated @types/mui-datatables?

@Volutionn
Copy link

Thanks for this upgrade!

@ozzyonfire
Copy link

I'm also having trouble with SSR using this version #1806

@Bhuvanesh-git331
Copy link

@zxhmike Will MUI-Datatable be compatable with MUI 5.2.3 v? because we are using mui 5.2.3 v on our project and planning use the MUI-Datatable. So throw some light on this.

Also I have shared my code sandbox link: https://codesandbox.io/s/mui-datatable-example-forked-7lu7ue?file=/src/App.js for reference.

Thanks.

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