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

Filter Table button crashes the page on Material-UI Next. #1559

Open
mburakeker opened this issue Oct 22, 2020 · 19 comments
Open

Filter Table button crashes the page on Material-UI Next. #1559

mburakeker opened this issue Oct 22, 2020 · 19 comments

Comments

@mburakeker
Copy link

Expected Behavior

I expect the Filter Table pop up to show up when I click to the button.

Current Behavior

It crashes whole the page.

Steps to Reproduce (for bugs)

https://codesandbox.io/s/muidatatables-custom-toolbar-forked-eff5p?file=/package.json

  1. Open CodeSandbox link
  2. Click the Filter Table button

Your Environment

Tech Version
Material-UI 5.0.0-alpha.12
MUI-datatables 3.5.0
React 16.13.1
browser CodeSandbox
@emotion/core 10.0.35
@emotion/styled 10.0.27
react-dom 16.13.1
@material-ui/core 5.0.0-alpha.12
@material-ui/icons 4.9.1
@mburakeker mburakeker changed the title Filter Table button crashes the page. Filter Table button crashes the page on Material-UI Next. Oct 22, 2020
@nickraphael
Copy link

Can confirm. This is killing me. Could do with some indication on when this could be resolved. It's holding up a production release. I've tried digging around but I've no idea.

@patorjk
Copy link
Collaborator

patorjk commented Oct 26, 2020

I haven't had a chance to test the library with version 5.0 yet. If anyone wants to take a look I'd be open to a PR for this as long as it's backwards compatible with Material UI 4.x.

@davidcraddock
Copy link

davidcraddock commented Oct 29, 2020

I have looked into this error. It is a breaking change from Material-UI version 4 to 5. They have renamed GridList to ImageList and GridListItem to ImageListItem along with changing tile to root in ImageListItem. Changing this in TableFilter.js fixes the problem.

mui/material-ui#22363
mui/material-ui#22385

So not sure a backwards compatible change is possible. I have forked the latest release in my own repository and have put the fix there and have attach the tableFilter.js file and the package.json file with the updated Material-UI version I used 5.0.0-alpha.14

https://github.com/davidcraddock/mui-datatables

fix.zip

@wdh2100
Copy link
Collaborator

wdh2100 commented Oct 29, 2020

@davidcraddock

It looks like GridList(ImageList) should be changed to Grid.

https://material-ui.com/components/grid/
https://material-ui.com/components/grid-list/

If there is no problem even if the component changes, I think it is correct to change.

What do you think?

@davidcraddock
Copy link

@wdh2100 I really don't mind how the final solution is architected, I just needed this problem solved quickly for a project I am working on and thought I would share what I did.

FYI to add it to your project replace the version of mui-datatables in your package.json with
"mui-datatables": "davidcraddock/mui-datatables"

and run npm update mui-datatables

@wdh2100
Copy link
Collaborator

wdh2100 commented Oct 29, 2020

I really don't mind how the final solution is architected, I just needed this problem solved quickly for a project I am working on and thought I would share what I did.

Yes, that's right.
thank you for share.

@wdh2100
Copy link
Collaborator

wdh2100 commented Oct 29, 2020

mui/material-ui#22900

Rename onChangeRowsPerPage, onChangePage

onChangePage={handlePageChange}
onChangeRowsPerPage={handleRowChange}

@davidcraddock
Copy link

I've updated my repo with the rename changes now as well.

Also onExited event no longer exists in Popover so I had to modify that to get it to work when you have an apply filter button

@destegabry
Copy link

destegabry commented Nov 2, 2020

Thank you @davidcraddock, saved me a fork!

@patorjk would you consider opening a next branch to consolidate all the changes needed for material-ui@5?

@destegabry
Copy link

@davidcraddock I'm using your fork in my project and it works perfectly when I run and build it from my local machine, but it breaks my CI.

It seems that npm ci drops the dist folder. Do you have any workaround for this?

@davidcraddock
Copy link

hmmm could be that my package.json and package-lock.json were out of sync and npm ci uses the package-lock.json file. I have updated it in my fork now, so see if that works. If not check that you are not missing an external dependency that mui-datatables rely on?

react
prop-types
lodash
clsx
react-dnd
react-dnd-html5-backend
react-to-print
@material-ui/core

@davidcraddock
Copy link

Updated fork to incorporate changes from #1565

@wdh2100 wdh2100 mentioned this issue Nov 3, 2020
@destegabry
Copy link

destegabry commented Nov 3, 2020

@davidcraddock seems nothing has changed :(

This is what npm ls outputs after npm install:

rm -Rf node_modules && npm install && npm ls  mui-datatables
└── mui-datatables@3.6.0  (github:davidcraddock/mui-datatables#d5edac7b56d8eed103a2c731849c6afef26df387)

This is after npm ci:

rm -Rf node_modules && npm ci && npm ls  mui-datatables
└── UNMET DEPENDENCY mui-datatables@github:davidcraddock/mui-datatables#d5edac7b56d8eed103a2c731849c6afef26df387 

@patorjk
Copy link
Collaborator

patorjk commented Nov 3, 2020

@destegabry have you tried 3.7.1? It was released last night and should have the needed changes in it.

@davidcraddock
Copy link

@patorjk I've just tests release 3.7.1 with Material-UI v5 and sadly it still does not work.

@destegabry looks like an UNMET DEPENDENCY, take a look at what versions of the dependenciess you are using in your project and compare them to what this package.json needs. For @material-ui/core I am using "^5.0.0-alpha.14" make sure you are using the same

@wdh2100
Copy link
Collaborator

wdh2100 commented Nov 4, 2020

@davidcraddock @patorjk

Changed in compatibility(v5) in 3.7.1.
However, there are still a few changes.

There are some changes in v5.
What I found and what you reported.

mui/material-ui#20012

There seems to be more.
It looks like more testing is needed.
It will not be easy to support both versions(v4, v5)

@destegabry
Copy link

@patorjk v3.7.1 it's not compatible with v5.0.0-alpha.12+ because of the changes that @wdh2100 have linked. I think it's not possible to have backward compatibility with ui-material v4.

@davidcraddock sorry but yesterday I didn't had time to check, this is my package.json, still getting UNMET DEPENDENCY only after npm ci:

{...
"dependencies": {
    "@datadog/browser-logs": "^1.25.0",
    "@date-io/date-fns": "^2.10.6",
    "@emotion/core": "^10.1.0",
    "@emotion/styled": "^10.0.27",
    "@material-ui/core": "^5.0.0-alpha.14",
    "@material-ui/icons": "^5.0.0-alpha.14",
    "@material-ui/lab": "^5.0.0-alpha.14",
    "@material-ui/pickers": "^4.0.0-alpha.12",
    "@material-ui/styles": "^5.0.0-alpha.14",
    "@top-solution/mui-inputs": "^0.9.0",
    "@top-solution/use-form": "^0.6.0",
    "@top-solution/utils": "^0.3.1",
    "axios": "^0.21.0",
    "clsx": "^1.1.1",
    "date-fns": "^2.16.1",
    "lodash.assignwith": "^4.2.0",
    "lodash.clonedeep": "^4.5.0",
    "lodash.debounce": "^4.0.8",
    "lodash.find": "^4.6.0",
    "lodash.get": "^4.4.2",
    "lodash.isequal": "^4.5.0",
    "lodash.isundefined": "^3.0.1",
    "lodash.memoize": "^4.1.2",
    "lodash.merge": "^4.6.2",
    "material-ui-chip-input": "^2.0.0-beta.2",
    "mdi-material-ui": "^6.20.0",
    "mui-datatables": "davidcraddock/mui-datatables",
    "prop-types": "^15.7.2",
    "react": "^17.0.1",
    "react-dnd": "^11.1.3",
    "react-dnd-html5-backend": "^11.1.3",
    "react-dom": "^17.0.1",
    "react-helmet": "^6.1.0",
    "react-redux": "^7.2.2",
    "react-router-dom": "^5.2.0",
    "react-sortable-tree": "^2.7.1",
    "react-to-print": "^2.8.0",
    "react-scripts": "^4.0.0",
    "redux": "^4.0.5",
    "typeface-roboto": "1.1.13",
    "yup": "^0.29.3"
  },
  "devDependencies": {
    "@types/mui-datatables": "^3.4.4",
    "@types/node": "^14.14.6",
    "@types/prop-types": "^15.7.3",
    "@types/react": "^16.9.55",
    "@types/react-dom": "^16.9.9",
    "@types/react-helmet": "^6.1.0",
    "@types/react-redux": "^7.1.9",
    "@types/react-router-dom": "^5.1.6",
    "@types/yup": "^0.29.9",
    "@typescript-eslint/parser": "^4.6.0",
    "eslint-config-prettier": "^6.15.0",
    "eslint-plugin-prettier": "^3.1.4",
    "eslint-plugin-react-hooks": "^4.2.0",
    "husky": "^4.3.0",
    "lint-staged": "^10.5.1",
    "prettier": "^2.1.2",
    "redux-devtools-extension": "^2.13.8",
    "typescript": "^4.0.5"
  },
...
}

@davidcraddock
Copy link

@destegabry I left @material-ui/icons to its default version of "@material-ui/icons": "^4.0.0", as you are using "@material-ui/icons": "^5.0.0-alpha.14" maybe that's the issue? try reverting that back and test otherwise compare whats in my forked package.json to your own and do some testing. I tried an npm ci on my project yesterday and it worked for me

@destegabry
Copy link

@davidcraddock I'm still confused but running npm config set unsafe-perm true have saved my day

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants