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

[material-next][theme] Move ref palette out of color schemes #40341

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

DiegoAndai
Copy link
Member

@DiegoAndai DiegoAndai commented Dec 27, 2023

Move the ref.palette out of colorSchemes as it doesn't depend on the color mode (light | dark). Having it inside the colorSchemes.light might confuse users. The change in usage is the following:

 const customTheme = extendTheme({
-  colorSchemes: {
-   light: {
       ref: {
         palette: {
           // ...
         },
       },
-     },
-  },
 });

@DiegoAndai DiegoAndai added the customization: theme Centered around the theming features label Dec 27, 2023
@DiegoAndai DiegoAndai self-assigned this Dec 27, 2023
@mui-bot
Copy link

mui-bot commented Dec 27, 2023

Netlify deploy preview

https://deploy-preview-40341--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 626ff72

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.

If I am not mistaken, this was initially defined like this, because people may want to define different palettes for light and dark mode, although the default Material You theme does not need it. How about we accept both and have a documentation about the different use-cases?

Edit: On the other hand, we can go initially with the minimal necessary API and see if it is needed based on the feedback.

@DiegoAndai
Copy link
Member Author

@mnajdova I think that's a valid concern.

people may want to define different palettes for light and dark mode

Naming might be confusing here. In v5 we use different palettes for different color modes. In Material You, the tokens for this use case are the system tokens. These are even named similarly to v5's palette tokens: primary, secondary, error, etc. On the other hand, making the names confusing, Material You's reference palette is not supposed to change based on context, as they're reference tokens.

So, if people want to create tokens that change depending on the color mode, they should do it with system tokens, not reference tokens. This keeps the users in line with how the design system was designed. It will also mean our implementation is more in line with that, making it more predictable.

This is how material-web has it implemented as well: system tokens have different values for light and dark modes, but reference tokens do not.

How about we accept both and have a documentation about the different use-cases?

Edit: On the other hand, we can go initially with the minimal necessary API and see if it is needed based on the feedback.

I agree with the edited part; start with what the design system defined and don't go outside unless we have a good reason for it.

@mnajdova
Copy link
Member

Agree! Likely it won't be needed, the palette itself should stay the same 👍

@DiegoAndai DiegoAndai merged commit bf891fe into mui:master Dec 29, 2023
22 checks passed
@DiegoAndai DiegoAndai deleted the next-extend-theme-ref-palette branch December 29, 2023 13:47
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants