[Theme] Restructure component definitions#22293
Conversation
packages/material-ui/src/styles/transformDeprecatedThemeFormat.js
Outdated
Show resolved
Hide resolved
|
@material-ui/core: parsed: +0.18% , gzip: +0.19% |
|
Something is off with the apply density button https://deploy-preview-22293--material-ui.netlify.app/customization/density/#main-content |
| props?: ComponentsProps['MuiAlert']; | ||
| variants?: Variants['MuiAlert']; | ||
| overrides?: Overrides['MuiAlert']; |
There was a problem hiding this comment.
I wonder about the naming convention. It could be
| props?: ComponentsProps['MuiAlert']; | |
| variants?: Variants['MuiAlert']; | |
| overrides?: Overrides['MuiAlert']; | |
| props?: Props['MuiAlert']; | |
| variants?: Variants['MuiAlert']; | |
| overrides?: Overrides['MuiAlert']; |
or
| props?: ComponentsProps['MuiAlert']; | |
| variants?: Variants['MuiAlert']; | |
| overrides?: Overrides['MuiAlert']; | |
| props?: ComponentsProps['MuiAlert']; | |
| variants?: ComponentsVariants['MuiAlert']; | |
| overrides?: ComponentsOverrides['MuiAlert']; |
Maybe the latter?
There was a problem hiding this comment.
Also, I was expecting relative imports, it seems strange to have these values coming from the core and not the lab.
There was a problem hiding this comment.
We are doing module augmentation and the variants are just inherited from the ComponentProps I think, so everything works. But I agree it looks confusing, let me see how can it be defined better.
There was a problem hiding this comment.
Adding Components prefix makes sense 👍
There was a problem hiding this comment.
Also, I was expecting relative imports, it seems strange to have these values coming from the core and not the lab.
@oliviertassinari the ComponentsProps, ComponentsVariants and ComponentsOverrides are all created based on the underlying mappings for the components. In the lab we are extending this underlying components mappings, so basically these typings are automatically updated, that is why we are importing from the core. Should we change how we are augmenting these typings?
// This is automatically updated
export type ComponentsOverrides = {
[Name in keyof ComponentNameToClassKey]?: Partial<StyleRules<ComponentNameToClassKey[Name]>>;
} & {
MuiCssBaseline?: {
'@global'?: {
'@font-face'?: CSSProperties['@font-face'];
} & Record<string, CSSProperties['@font-face'] | CSSProperties>; // allow arbitrary selectors
};
};
// We are overriding these mappings
export interface ComponentNameToClassKey {
MuiAppBar: AppBarClassKey;
MuiAvatar: AvatarClassKey;
MuiBackdrop: BackdropClassKey;
// ...
}
Good catch! Fixed |
|
@joshwooding we might be able to add the theme.mixins.gutter with this new |
oliviertassinari
left a comment
There was a problem hiding this comment.
All good from my end. I have made a few small proposals in the last commit:
- Make
adaptV4Themedeprecated so developers don't forget to remove it. - Group the Theme changes under the same section of the migration guide.
- Remove mention to theme.variants in the upgrade from v4 guide so developers don't get confused if they can't find it (I'm not entirely sure that it's better)
Great work!
Co-authored-by: Matt <github@nospam.33m.co>
Closes #21923
BREAKING CHANGE
createMuiTheme
componentskey, to allow people easier discoverability about the definitions regarding one component.propsimport { createMuiTheme } from '@material-ui/core/styles'; const theme = createMuitheme({ - props: { - MuiButton: { - disableRipple: true, - }, - }, + components: { + MuiButton: { + props: { + disableRipple: true, + }, + }, + }, });overridesimport { createMuiTheme } from '@material-ui/core/styles'; const theme = createMuitheme({ - overrides: { - MuiButton: { - root: { padding: 0 }, - }, - }, + components: { + MuiButton: { + overrides: { + root: { padding: 0 }, + }, + }, + }, });For a smoother transition, the
adaptV4Themehelper allows you to iteratively upgrade to the new theme structure. Note that it will display a deprecation warning in the console, since it will be removed at the next major release.