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

[POC] Switch between MD2 and MD3 #42904

Closed
wants to merge 7 commits into from

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jul 11, 2024

This PR experiments how we can add support for MD3 while keeping MD2 alongside. The idea is to provide two themes, and for now keep MD2 by default. In v7 we can switch the default to MD3.

In order to make sure we won't break the previous users' style overrides, we can isolate the styles for MD3 completely by providing different slots components. Each component can use the useIsMD3 hook, and based on it use the correct styled component (see the Button implementation in the PR).

Preview: https://deploy-preview-42904--material-ui.netlify.app/experiments/md3/

@mnajdova mnajdova added the proof of concept Studying and/or experimenting with a to be validated approach label Jul 11, 2024
@mui-bot
Copy link

mui-bot commented Jul 11, 2024

Netlify deploy preview

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

Bundle size report

Bundle size will be reported once CircleCI build #719140 finishes.

Generated by 🚫 dangerJS against 0234312

@@ -345,12 +345,323 @@ const ButtonEndIcon = styled('span', {
],
}));

export const ButtonRootMD3 = styled(ButtonBase, {
Copy link
Member Author

Choose a reason for hiding this comment

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

These are just copied from the previous @mui/material-next package, they would need to be converted to support Pigment CSS.

@siriwatknp
Copy link
Member

I like the idea of switching the slots. It seems to be the viable method without huge breaking change. However, I don't agree with using useIsMd3 to check inside each component because it will increase the bundle size significantly (non tree-shaking) and I think it's hard to maintain in the future.

Instead I think we have to extract specific MD2 styles out of the component to MD2Theme, meaning the default Button will display as plain button.

import Button from '@mui/material/Button';

<Button>test</Button> // this button will be a plain button without MD2 specific, like Ripple.

Then create a theme and slots for MD2 and MD3:

import { extendMd2Theme, Md2Slots } from '@mui/material/md2';

<CssVarsProvider theme={extendTheme()}>
  <DefaultPropsProvider value={MD2Slots}>
     …app
  </DefaultPropsProvider>
</CssVarsProvider>

The MD2Slots could look like this:

{ MuiButton: { defaultProps: { slots: { ripple: TouchRipple } }} }

Same for MD3:

import { extendMd3Theme, Md3Slots } from '@mui/material/md3';

<CssVarsProvider theme={extendMd3Theme()}>
  <DefaultPropsProvider value={Md3Slots}>
     …app
  </DefaultPropsProvider>
</CssVarsProvider>
{ MuiButton: { defaultProps: { slots: { ripple: null } }} }

It's quite similar to MUI X Data Grid slots override but I need to do a POC first.

@mnajdova
Copy link
Member Author

I would advocate for keeping this internal details, regardless of how we implement it internally, slots, theme etc, the end-user experience should be that they need to change the theme. Let's create a POC, we can hide the implementation details about the default props as part of the theme.

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

I am closing this experiment, we'll have it anyway if wee need to come back at it.

@mnajdova mnajdova closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged proof of concept Studying and/or experimenting with a to be validated approach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants