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] Fix Hidden breakpoints issues and updates the migration guide #22702

Merged
merged 11 commits into from
Sep 24, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Sep 23, 2020

This PR addresses #22695 (comment) and updates the adaptV4Theme utility to support the type palette prop. In addition the migration-v4.md theme breaking changes is updated.

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 23, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 7c5c413

@oliviertassinari
Copy link
Member

  1. So If I understand correctly, the only way to bring the v4 breakpoint behavior would be to have all internal usages flag that they are using the new variant. This might be overkill.
  2. I don't understand why adaptV4Theme isn't enough to handle the theme.palette.type vs theme.palette.mode problem.

@mnajdova
Copy link
Member Author

  1. So If I understand correctly, the only way to bring the v4 breakpoint behavior would be to have all internal usages flag that they are using the new variant. This might be overkill.

Correct, I think we should just document this as a breaking change that we cannot backport..

  1. I don't understand why adaptV4Theme isn't enough to handle the theme.palette.type vs theme.palette.mode problem.

For the input we are good, people can specify it as adaptV4Theme({ palette: { type: 'dark } }) but then in the styles, it is available trough theme.palette.mode, not theme.palette.type anymore - we could add both in the palette and it won't require any changes on client's side...

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 23, 2020

Correct, I think we should just document this as a breaking change that we cannot backport..

Happy to do that.

but then in the styles, it is available trough theme.palette.mode, not theme.palette.type anymore

Ok I see. What about we have adaptV4Theme() always return type and mode and force the default value? Would it solve the problem?

diff --git a/packages/material-ui/src/styles/adaptV4Theme.js b/packages/material-ui/src/styles/adaptV4Theme.js
index bc0adac56e..c67386fcbc 100644
--- a/packages/material-ui/src/styles/adaptV4Theme.js
+++ b/packages/material-ui/src/styles/adaptV4Theme.js
@@ -74,17 +74,20 @@ export default function adaptV4Theme(inputTheme) {
     ...mixins,
   };

-  const { type: mode, ...paletteRest } = palette;
+  const { type, mode, ...paletteRest } = palette;
+
+  const finalMode = mode || type || 'light';

   // theme.palette.text.hint
   theme.palette = {
     text: {
       hint:
-        palette.mode === 'dark' || palette.type === 'dark'
+        finalMode === 'dark'
           ? 'rgba(255, 255, 255, 0.5)'
           : 'rgba(0, 0, 0, 0.38)',
     },
-    mode,
+    mode: finalMode,
+    type: finalMode,
     ...paletteRest,
   };

@oliviertassinari
Copy link
Member

I guess we could also handle #22695 (comment) here too O:).

@mnajdova
Copy link
Member Author

Agree, let me do the updates and fix the Hidden component 👍

@mnajdova mnajdova changed the title [theme] Add createMuiV4Theme utility [theme] Fix Hidden breakpoints issues and updates the migration guide Sep 23, 2020
@mnajdova
Copy link
Member Author

@oliviertassinari changes are implemented, please take another look :)

mnajdova and others added 5 commits September 23, 2020 18:17
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mnajdova mnajdova requested a review from mbrookes September 23, 2020 20:04
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 23, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 24, 2020
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 24, 2020
@mnajdova mnajdova merged commit 1c2055a into mui:next Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants