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

[core] Remove makeStyles from @material-ui/core #26382

Merged
merged 11 commits into from
May 21, 2021

Conversation

mnajdova
Copy link
Member

BREAKING CHANGE

  • The makeStyles JSS utility is no longer exported from @material-ui/core/styles. You can use @material-ui/styles/makeStyles instead. Make sure to add a ThemeProvider at the root of your application, as the defaultTheme is no longer available. If you are using this utility together with @material-ui/core, it's recommended you use the ThemeProvider component from @material-ui/core/styles instead.

    -import { makeStyles } from '@material-ui/core/styles';
    +import { makeStyles } from '@material-ui/styles';
    +import { createTheme, ThemeProvider } from '@material-ui/core/styles';
    
    +const theme = createTheme();
     const useStyles = makeStyles((theme) => ({
       background: theme.palette.primary.main,
     }));
     function Component() {
       const classes = useStyles();
       return <div className={classes.root} />
     }
    
     // In the root of your app
     function App(props) {
    -  return <Component />;
    +  return <ThemeProvider theme={theme}><Component {...props} /></ThemeProvider>;
     }

@mnajdova mnajdova marked this pull request as draft May 19, 2021 16:37
@mui-pr-bot
Copy link

mui-pr-bot commented May 19, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 95bba03

@mnajdova
Copy link
Member Author

@dtassone we have this regression - https://www.argos-ci.com/mui-org/material-ui/builds/3599 after upgrading to @material-ui/[email protected], is it expected? I assume is because of the new sorting icon. Maybe we should increase the width of the table in this example.

@mnajdova
Copy link
Member Author

@eps1lon @oliviertassinari what shall we do with notistack still using makeStyles? Our deploy is failing - https://app.netlify.com/sites/material-ui/deploys/60a5471966f35f0007ce0549

@mnajdova mnajdova marked this pull request as ready for review May 19, 2021 21:27
@@ -31,7 +31,7 @@
"@fortawesome/free-solid-svg-icons": "^5.14.0",
"@fortawesome/react-fontawesome": "^0.1.11",
"@material-ui/core": "5.0.0-alpha.34",
"@material-ui/data-grid": "^4.0.0-alpha.21",
"@material-ui/data-grid": "^4.0.0-alpha.29",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed, as this version does not use makeStyles from @material-ui/core/styles anymore.

@@ -18,7 +18,9 @@ const columns = [
sortable: false,
width: 160,
valueGetter: (params) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`,
`${params.getValue(params.id, 'firstName') || ''} ${
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a breaking change.

@@ -1617,8 +1642,6 @@ As the core components use emotion as a styled engine, the props used by emotion
}
```

Note: If you would like to move
Copy link
Member Author

Choose a reason for hiding this comment

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

Cleanup, was some leftover forgotten

@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2021

what shall we do with notistack still using makeStyles? Our deploy is failing

@mnajdova

@mnajdova
Copy link
Member Author

Happy to go with whatever is the best. Option 1 seems to allow us to move fastest, but I am ok with any of the options. We could disable the example now and re-enable once notistack is v5 compatible too.

@dtassone
Copy link
Member

@dtassone we have this regression - https://www.argos-ci.com/mui-org/material-ui/builds/3599 after upgrading to @material-ui/[email protected], is it expected? I assume is because of the new sorting icon. Maybe we should increase the width of the table in this example.

Yes but looking at it now, I don't think it's great. We have largely enough space to render the title, it looks empty, and you wonder why the col title is not there or is cut as in the image below.
image

@mnajdova
Copy link
Member Author

Yes but looking at it now, I don't think it's great. We have largely enough space to render the title, it looks empty, and you wonder why the col title is not there or is cut as in the image below.

Agree, maybe would be better if the toolbar (the icons) is rendered as some popover

@mnajdova
Copy link
Member Author

@dtassone I've increased the width of the columns by 20, it looks better now:

image

We can update the example with the next release once this is fixed.

@mnajdova mnajdova requested a review from oliviertassinari May 20, 2021 11:05
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Looks good

docs/src/pages/components/snackbars/snackbars.md Outdated Show resolved Hide resolved
docs/src/pages/components/tables/DataTable.tsx Outdated Show resolved Hide resolved
@eps1lon
Copy link
Member

eps1lon commented May 20, 2021

@eps1lon @oliviertassinari what shall we do with notistack still using makeStyles?

Ideally just rewrite an import if that's possible. It's a good test whether the migration story is viable.

@mnajdova
Copy link
Member Author

Ideally just rewrite an import if that's possible. It's a good test whether the migration story is viable.

Rewrite import + add defaultTheme is the way to go. I will create a PR there, but would not block this by the changes there. Added Todo to enable it once this is done.

@eps1lon
Copy link
Member

eps1lon commented May 20, 2021

add defaultTheme is the way to go

Where does @material-ui/styles get the theme from? We can add a their provider to our docs as an interim solution.

@mnajdova
Copy link
Member Author

Where does @material-ui/styles get the theme from? We can add a their provider to our docs as an interim solution.

I am creating it with createTheme from core. The problem is that in their repo, they are importing makeStyles from @material-ui/core/styles, but that function is removed. The import needs to be changed too.

@eps1lon
Copy link
Member

eps1lon commented May 20, 2021

@material-ui/core/styles, but that function is removed. The import needs to be changed too.

We can rewrite this with babel in our repo. We're already doing a bunch of rewrites:

https://github.com/eps1lon/material-ui/blob/323b76bec7391d7c95954096814d6876a3813c77/docs/next.config.js#L103-L138

So it sounds like we should be able to add the new one for compat if that is all required for migrating

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 20, 2021
@mnajdova
Copy link
Member Author

We can rewrite this with babel in our repo. We're already doing a bunch of rewrites:

https://github.com/eps1lon/material-ui/blob/323b76bec7391d7c95954096814d6876a3813c77/docs/next.config.js#L103-L138

So it sounds like we should be able to add the new one for compat if that is all required for migrating

How can we do this, if only some of the imports from a package should be changed, for example:

-import { makeStyles } from '@material-ui/core/styles';
+import { makeStyles } from '@material-ui/styles';

// but these should still be imported from @material-ui/core/styles
import { Theme, ThemProvider } from '@material-ui/core/styles';

@eps1lon
Copy link
Member

eps1lon commented May 20, 2021

sigh yeah, nevermind. The migration story is just all over the place unfortunately.

@mnajdova mnajdova requested a review from oliviertassinari May 21, 2021 13:48
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 21, 2021
@mnajdova mnajdova merged commit 2ec7536 into mui:next May 21, 2021
@oliviertassinari oliviertassinari changed the title [core] Remove makeStyles from @material-ui/core/styles [core] Remove makeStyles from @material-ui/core May 29, 2021
@oliviertassinari oliviertassinari added this to the v5-beta milestone Aug 17, 2021
@oliviertassinari oliviertassinari mentioned this pull request Aug 17, 2021
42 tasks
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.

5 participants