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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Fix support for @material-ui/core@4.12 #2108

Merged
merged 7 commits into from
Jul 16, 2021
Merged

[DataGrid] Fix support for @material-ui/core@4.12 #2108

merged 7 commits into from
Jul 16, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jul 9, 2021 •
edited by oliviertassinari
Loading

@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jul 9, 2021
@DanailH DanailH requested a review from a team July 9, 2021 08:59
@DanailH DanailH self-assigned this Jul 9, 2021
oliviertassinari
oliviertassinari previously requested changes Jul 9, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

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

"Update isMuiV5 helper" assuming what we go after is "Fix support for @material-ui/core@4.12", we also need to update muiStyleAlpha and createTheme to remove the deprecation warnings. We can no longer test the version, we need to feature detect.

@oliviertassinari oliviertassinari mentioned this pull request Jul 9, 2021
Closed
@DanailH
Copy link
Member Author

DanailH commented Jul 9, 2021 •
edited
Loading

@oliviertassinari Yes, I had a quick chat with @mnajdova this morning about it that was the easiest solution to the problem.

we also need to update muiStyleAlpha and createTheme to remove the deprecation warnings.

The drawback with doing that is that we need to also update the core version we support. Basically to use the grid one will need to use core@4.12.0 > , no ?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 9, 2021 •
edited
Loading

The drawback with doing that is that we need to also update the core version we support. Basically to use the grid one will need to use core@4.12.0 > , no ?

No need to change the support version, we can use feature detection.

@DanailH DanailH changed the title [DataGrid] Update isMuiV5 helper [DataGrid] Fix support for @material-ui/core@4.12 Jul 9, 2021
@DanailH
Copy link
Member Author

DanailH commented Jul 9, 2021

The tests are failing because of depreciation warnings. Basically now we need a way to know if it is v4.12 because I don't see how we can detect things like <TablePagination onChangePage /> or <TablePagination onPageChange />

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 9, 2021 •
edited
Loading

@DanailH Arf, maybe we should do what you have proposed initially then, drop <v4.12 versions support? I can't think of any way to know if we are on v4.12 or v4.11. Maybe someone else will.

@oliviertassinari oliviertassinari force-pushed the bug/DataGrid-2106-isMuiV5-not-working branch from c0961c1 to 5e702eb Compare July 10, 2021 13:58
@oliviertassinari oliviertassinari dismissed their stale review July 10, 2021 14:29

I have fixed the build, as turns out, we could easily detect v4.12.1

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jul 10, 2021
oliviertassinari
oliviertassinari reviewed Jul 10, 2021
View reviewed changes
packages/grid/_modules_/grid/utils/utils.ts
}

export function muiStyleAlpha(color: string, value: number): string {
if (isMuiV5()) {
if ((styles as any)?.alpha) {
Copy link
Member

@oliviertassinari oliviertassinari Jul 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

A good take away for us: feature detection >> version matching.

@oliviertassinari oliviertassinari mentioned this pull request Jul 10, 2021
Merged
1 task
@m4theushw m4theushw mentioned this pull request Jul 13, 2021
Merged
1 task
@oliviertassinari oliviertassinari force-pushed the bug/DataGrid-2106-isMuiV5-not-working branch from 65432b8 to 4cf8a7b Compare July 14, 2021 22:48
@oliviertassinari oliviertassinari removed the request for review from flaviendelangle July 14, 2021 22:48
@oliviertassinari oliviertassinari merged commit 6a14c48 into mui:master Jul 16, 2021
@bartvde
Copy link

bartvde commented Jul 19, 2021

any chance there will be a release soon that has this fix incorporated? We are running into:

Warning: Failed prop type: Invalid prop `modifiers` of type `array` supplied to `ForwardRef(Popper)`, expected `object`.

because of this one. TIA.

@striky1
Copy link

striky1 commented Jul 20, 2021 •
edited
Loading

any chance there will be a release soon that has this fix incorporated? We are running into:

Warning: Failed prop type: Invalid prop `modifiers` of type `array` supplied to `ForwardRef(Popper)`, expected `object`.

because of this one. TIA.

Same here + I still have errors:

index.js:1 Material-UI: The key `selectLabel` provided to the classes prop is not implemented in ForwardRef(TablePagination).
You can only override one of the following: root,toolbar,spacer,caption,selectRoot,select,selectIcon,input,menuItem,actions.

I tried downgrade back of /core to 4.11.4 but it didn't help.

Current package.json:

    "@material-ui/core": "^4.12.2",
    "@material-ui/data-grid": "^4.0.0-alpha.33",
    "@material-ui/icons": "^4.11.2",
    "@material-ui/lab": "^4.0.0-alpha.58",
    "@material-ui/pickers": "^3.3.10",
    "@material-ui/styles": "^4.11.4",

Thank you @oliviertassinari

@Primajin
Copy link 

    "@material-ui/core": "^4.12.2",
    "@material-ui/icons": "^4.11.2",
    "@material-ui/lab": "4.0.0-alpha.60",
    "@material-ui/pickers": "^3.3.10",
    "@material-ui/x-grid": "^4.0.0-alpha.34",

seems to have resolved the issue

@striky1
Copy link

striky1 commented Jul 22, 2021 

    "@material-ui/core": "^4.12.2",
    "@material-ui/icons": "^4.11.2",
    "@material-ui/lab": "4.0.0-alpha.60",
    "@material-ui/pickers": "^3.3.10",
    "@material-ui/x-grid": "^4.0.0-alpha.34",

seems to have resolved the issue

but you are using x-grid instead of data-grid what is not MIT version

@mui mui locked as resolved and limited conversation to collaborators Jul 22, 2021
@oliviertassinari
Copy link
Member

@striky1 He's a Pro plan user. This fix was released in MUI X version: 4.0.0-alpha.34, regardless of the plan (MIT or commercial).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@flaviendelangle flaviendelangle flaviendelangle left review comments

Assignees

@m4theushw m4theushw

Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! priority: important This change can make a difference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DataGrid] Broken support of core v4.12.1
7 participants
@DanailH @oliviertassinari @bartvde @striky1 @Primajin @flaviendelangle @m4theushw