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

[theme] Rename createMuiTheme to createTheme #25992

Merged
merged 6 commits into from
Apr 29, 2021

Conversation

m4theushw
Copy link
Member

@m4theushw m4theushw commented Apr 26, 2021

Depreciations

  • [theme] Rename createMuiTheme to createTheme.
    Developers only need one theme in their application. A prefix would suggest a second theme is needed. It's not the case. createMuiTheme will be removed in v6.
-import { createMuiTheme } from '@material-ui/core/styles';
+import { createTheme } from '@material-ui/core/styles';

-const theme = createMuiTheme({
+const theme = createTheme({

Part of #20012.

Sorry for the huge PR. 😅 I did a search & replace for createMuiTheme ignoring everything from the blog, CHANGELOG and migration guides of previous versions. I didn't rename variables names: muiTheme -> theme.

Should we rename unstable_createMuiStrictModeTheme?

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 26, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 5a44e9a

@oliviertassinari
Copy link
Member

Should we rename unstable_createMuiStrictModeTheme?

This is for v4. I believe it's no longer relevant anymore cc @eps1lon. It looks like another breaking change we should do.

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

eps1lon commented Apr 27, 2021

This is for v4. I believe it's no longer relevant anymore cc @eps1lon. It looks like another breaking change we should do.

Let's keep it for now since there are more StrictMode changes coming in React core. We can make it a no-op though or rather just an alias for createMuiTheme i.e. keep the interface but simplify the implementation.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 27, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 27, 2021

Let's keep it for now since there are more StrictMode changes coming in React core. We can make it a no-op though or rather just an alias for createMuiTheme i.e. keep the interface but simplify the implementation.

Actually, should we do the same with createMuiTheme()? I mean, do we really need to make this a BC in v5? It seems to impact a lot of people, the overhead for us to only remove it in v6 seems acceptable. Maybe we could do #26004 on v5 as well. It's more or less what we did for MuiThemeProvider -> ThemeProvider. We took a lot of time to make it happen. Thoughts?

@m4theushw
Copy link
Member Author

Actually, should we do the same with createMuiTheme()? I mean, do we really need to make this a BC in v5? It seems to impact a lot of people, the overhead for us to only remove it in v6 seems acceptable. Maybe we could do #26004 on v5 as well. It's more or less what we did for MuiThemeProvider -> ThemeProvider. We took a lot of time to make it happen. Thoughts?

I would rename it, but keeping createMuiTheme with a warning to use the new recommendation. This breaking change doesn't bring value for users and we already have other more important changes in v5 that will require attention.

@@ -44,7 +44,7 @@ yarn add @material-ui/core@next
```tsx
import '@material-ui/lab/themeAugmentation';

const theme = createMuiTheme({
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we are supposed to do changes to the translated files. @oliviertassinari ?

Copy link
Member

Choose a reason for hiding this comment

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

It's best to avoid it as it add noises in the pull requests. But since this one is already very noisy, no strong preferences.

Copy link
Member

Choose a reason for hiding this comment

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

Got ya!

@@ -47,7 +47,7 @@ async function createApp() {

let index = await fse.readFile(path.join(rootPath, 'examples/cdn/index.html'), 'utf8');
index = index.replace(
'https://unpkg.com/@material-ui/core@latest/umd/material-ui.development.js',
'https://unpkg.com/@material-ui/core@next/umd/material-ui.development.js',
Copy link
Member

Choose a reason for hiding this comment

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

how is this related?

Copy link
Member Author

@m4theushw m4theushw Apr 28, 2021

Choose a reason for hiding this comment

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

The replace was doing nothing, because the path in examples/cdn/index.html uses the next tag. Since I had removed createMuiTheme it broke the e2e tests. Now that we have createMuiTheme back it could be reverted, or we leave it but once v5 becomes stable we have to update it back to latest.

https://github.com/mui-org/material-ui/blob/d7c0ae5958a274c6d7043b3bda8291a1d93fd3c1/examples/cdn/index.html#L9

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks! Let’s leave it I’d say

Copy link
Member

@oliviertassinari oliviertassinari Apr 28, 2021

Choose a reason for hiding this comment

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

I would personally keep latest so we don't forget to update it.

On this CDN aspect, I was also eager to replace/complement this example with something more modern https://trello.com/c/8QGmlDLH

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I've scanned all files, this is the only non related change I could spot - #25992 (comment) I am going to create lots of conflicts with the PRs that removes the @material-ui/styles utils from core, so I'd say it's better if we can merge this soon :)

@oliviertassinari oliviertassinari added deprecation New deprecation message and removed breaking change labels Apr 28, 2021
@mnajdova
Copy link
Member

I am merging this one, not keep on resolving merge conflicts in 100+ files :D

@mnajdova mnajdova merged commit 16e5d03 into mui:next Apr 29, 2021
eps1lon pushed a commit to siriwatknp/material-ui that referenced this pull request Apr 29, 2021
@m4theushw m4theushw deleted the rename-createMuiTheme branch April 29, 2021 11:24
siriwatknp added a commit that referenced this pull request May 12, 2021
* migrate to emotion

* make styleProps optional

* remove lint and add fn name

* use yearButton from styles

* revert types

* [docs] Document all the colors available (#26015)

* [Timeline] Add support for position override on items (#25974)

* [core] Remove deprecated innerRef prop (#26028)

[core] Remove deprecated innerRef prop (#26028)

* [theme] Rename `createMuiTheme` to `createTheme` (#25992)

* [pickers] Remove redundant aria-hidden (#26014)

* [internal][pickers] Remove unused styles (#26023)

* [pickers] Toggle mobile keyboard view in the same commit as the view changes (#26017)

* [DateRangePicker] Fix not being opened on click (#26016)

* Inline classes

* type -> interface

* sort asc

* default props are private

* remove uneccesary type casting

* follow convention

* trigger pipeline

Co-authored-by: Anshuman Pandey <[email protected]>
Co-authored-by: simonecervini <[email protected]>
Co-authored-by: Matheus Wichman <[email protected]>
Co-authored-by: Sebastian Silbermann <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation New deprecation message
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants