Skip to content

[styles] Remove the old withStyles modules #14767

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
Merged
merged 3 commits into from
Mar 7, 2019
Merged

[styles] Remove the old withStyles modules #14767

merged 3 commits into from
Mar 7, 2019

Conversation

oliviertassinari
Copy link
Member

This pull request removes the legacy style modules. It's time to rely on @material-ui/styles. It's also re-exploring the style modules with a default theme to make the transition seamless.

The TypeScript definitions and the documentation are not correctly handled yet. I think that we can do it in a second pass. I'm not sure what drives the bundle size increase. It could be the JSS alpha.7 to alpha.12 upgrade.

Deprecation

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

Breaking changes

  • Remove the MuiThemeProvider component:
-import { MuiThemeProvider } from '@material-ui/core/styles';
+import { ThemeProvider } from '@material-ui/styles';
  • Remove the @material-ui/styles/install module.
-import { install } from '@material-ui/styles';

-install();

Closes #14416
Closes #14698

@oliviertassinari
[styles] Remove the old styles modules
1078da9 
* [styles] Remove the old styles modules

* reviews
@oliviertassinari oliviertassinari added breaking change package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Mar 6, 2019
@oliviertassinari oliviertassinari force-pushed the remove-old-styles-modules branch from 22c8147 to 1078da9 Compare March 6, 2019 13:50
@oliviertassinari oliviertassinari mentioned this pull request Mar 6, 2019
Merged
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 6, 2019 •
edited
Loading

@material-ui/core: parsed: -2.51% 😍, gzip: +0.14%
@material-ui/lab: parsed: -7.40% 😍, gzip: -1.05% 😍
@material-ui/styles: parsed: +2.57% , gzip: +2.70%

Details of bundle changes.

Comparing: 7a49f80...14356aa

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -2.51% +0.14% 🔺 371,832 362,507 91,939 92,069
@material-ui/core/Paper -10.27% +4.38% 🔺 76,648 68,773 19,308 20,153
@material-ui/core/Paper.esm -11.60% +2.61% 🔺 71,599 63,294 18,783 19,274
@material-ui/core/Popper -0.01% -0.24% 30,462 30,458 10,582 10,557
@material-ui/core/styles/createMuiTheme -0.01% -0.35% 17,286 17,284 5,717 5,697
@material-ui/core/useMediaQuery 0.00% 0.00% 2,486 2,486 1,049 1,049
@material-ui/lab -7.40% -1.05% 184,281 170,648 50,584 50,051
@material-ui/styles +2.57% 🔺 +2.70% 🔺 55,687 57,118 15,825 16,253
@material-ui/system -0.12% +0.36% 🔺 17,062 17,041 4,482 4,498
Button -8.35% +1.16% 🔺 99,409 91,106 26,607 26,916
Modal -8.86% +1.72% 🔺 98,984 90,214 26,263 26,716
colorManipulator 0.00% +0.15% 🔺 3,232 3,232 1,296 1,298
docs.landing 0.00% 0.00% 51,891 51,891 11,291 11,291
docs.main -7.81% -6.40% 678,326 625,369 206,184 192,994
packages/material-ui/build/umd/material-ui.production.min.js -3.53% +0.16% 🔺 322,466 311,092 85,044 85,183

Generated by 🚫 dangerJS against 14356aa

mbrookes
mbrookes reviewed Mar 6, 2019
View reviewed changes
docs/src/pages/css-in-js/basics/basics.md Outdated
@@ -10,13 +10,13 @@ You can use it, but you don't have to. This styling solution is [interoperable w

In previous versions, Material-UI has used LESS, then a custom inline-style solution to write the style of the
components, but these approaches have proven to be limited. Most recently, we have [moved toward](https://github.com/oliviertassinari/a-journey-toward-better-style)
a *CSS-in-JS* solution. It **unlocks many great features** (theme nesting, dynamic styles, self-support, etc.).
a *CSS in JS* solution. It **unlocks many great features** (theme nesting, dynamic styles, self-support, etc.).
Copy link
Member

Choose a reason for hiding this comment

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

Why changing? The hyphenated version seems to be widely used.

Copy link
Member Author

Choose a reason for hiding this comment

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

We were using the two variants. Try a Google search, everybody is contradicting itself. Ok, let's trust this variant: https://medium.com/seek-blog/a-unified-styling-language-d0c208de2660.
There are relatively few results when Google CSS-in-JS, what do you think of renaming it to simple "styling"?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://next.material-ui.com/css-in-js/basics/ would become https://next.material-ui.com/styles/basics/

Copy link
Member Author

Choose a reason for hiding this comment

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

as for https://next.material-ui.com/style/icons/, we can either rename it content, or move it to the components subsection.

mbrookes
mbrookes reviewed Mar 6, 2019
View reviewed changes
docs/src/pages/guides/interoperability/interoperability.md
@@ -7,7 +7,7 @@ but you should find that the principals applied here can be adapted to other lib

We have provided examples for the following styling solutions:

- [Raw CSS](#raw-css)
- [Plain CSS](#plain-css)
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@oliviertassinari oliviertassinari mentioned this pull request Mar 7, 2019
Closed
@oliviertassinari oliviertassinari merged commit 4267723 into mui:next Mar 7, 2019
@oliviertassinari oliviertassinari deleted the remove-old-styles-modules branch March 7, 2019 19:14
@oliviertassinari oliviertassinari mentioned this pull request Mar 10, 2019
Merged
@pheuter pheuter mentioned this pull request Mar 10, 2019
Closed
@synchronos-t
Copy link

Is @material-ui/core/styles/createMuiTheme() still the only (and correct) way to create customized themed based on default theme (i.e. without writing the whole theme from scratch)? There doesn't seem to be anything providing the same functionality in the Styles API documentation.

@oliviertassinari oliviertassinari changed the title [styles] Remove the old styles modules [styles] Remove the old withStyles modules Mar 11, 2019
@oliviertassinari
Copy link
Member Author

@synchronos-t Yes. Maybe we should split @material-ui/core/styles in two:

  • @material-ui/core/styles
  • @material-ui/core/createMuiTheme

@synchronos-t
Copy link

Is there any particular reason why the createMuiTheme resides under the core and not under styles? Unless it's because you've planned to just move it sometimes later. To me, it'd sound more intuitive that the theme creation functions are where the styles and theme usage functions are. (And probably now named createTheme, I assume.) Though, I have a hunch it might have to do something how the default theme is applied to all the components.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 12, 2019 •
edited
Loading

Is there any particular reason why the createMuiTheme resides under the core and not under styles?

Yes, @material-ui/styles is a standalone style solution, it doesn't know anything about the core components.

it'd sound more intuitive that the theme creation functions are where the styles and theme usage functions are.

It's why we have kept @material-ui/core/styles/withStyles: https://next.material-ui.com/customization/default-theme/#-material-ui-core-styles-vs-material-ui-styles.

@PeterL1n
Copy link

Without install() I cannot get the dark theme working. Also, changing borderRadius in the theme does not override the MUI component. The entire thing is broken. Am I missing something?

@oliviertassinari
Copy link
Member Author

@PeterL1n install() was removed in v4.0.0-alpha.3. Make sure the core and styles versions match.

@eps1lon eps1lon mentioned this pull request Mar 19, 2019
Closed
56 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrookes mbrookes mbrookes left review comments

@eps1lon eps1lon Awaiting requested review from eps1lon

@joshwooding joshwooding Awaiting requested review from joshwooding

Assignees
No one assigned
Labels
breaking change package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oliviertassinari @mui-pr-bot @synchronos-t @PeterL1n @mbrookes