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
[Joy] Improve variant customization experience #30878
[Joy] Improve variant customization experience #30878
Conversation
@material-ui/system: parsed: +0.44% , gzip: +0.59% |
…joy/variant-implementation
@@ -79,7 +79,7 @@ const ButtonRoot = styled('button', { | |||
padding: '0.25rem var(--Button-gutter)', // the padding-top, bottom act as a minimum spacing between content and root element | |||
...(ownerState.variant === 'outlined' && { | |||
padding: | |||
'calc(0.25rem - var(--variant-outlined-borderWidth)) calc(var(--Button-gutter) - var(--variant-outlined-borderWidth))', // account for the border width | |||
'calc(0.25rem - var(--variant-outlinedBorderWidth)) calc(var(--Button-gutter) - var(--variant-outlinedBorderWidth))', // account for the border width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to match the variant variables.
resolveTheme: (mergedTheme: JoyTheme) => { | ||
mergedTheme.variants = deepmerge( | ||
{ | ||
text: createVariant('text', mergedTheme), | ||
textHover: createVariant('textHover', mergedTheme), | ||
textActive: createVariant('textActive', mergedTheme), | ||
textDisabled: createVariant('textDisabled', mergedTheme), | ||
outlined: createVariant('outlined', mergedTheme), | ||
outlinedHover: createVariant('outlinedHover', mergedTheme), | ||
outlinedActive: createVariant('outlinedActive', mergedTheme), | ||
outlinedDisabled: createVariant('outlinedDisabled', mergedTheme), | ||
light: createVariant('light', mergedTheme), | ||
lightHover: createVariant('lightHover', mergedTheme), | ||
lightActive: createVariant('lightActive', mergedTheme), | ||
lightDisabled: createVariant('lightDisabled', mergedTheme), | ||
contained: createVariant('contained', mergedTheme), | ||
containedHover: createVariant('containedHover', mergedTheme), | ||
containedActive: createVariant('containedActive', mergedTheme), | ||
containedDisabled: createVariant('containedDisabled', mergedTheme), | ||
containedOverrides: createContainedOverrides(mergedTheme), | ||
} as typeof mergedTheme.variants, | ||
mergedTheme.variants, | ||
{ clone: false }, | ||
); | ||
return mergedTheme; | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change of this PR.
@@ -17,7 +16,7 @@ export default function ThemeProvider({ | |||
children, | |||
theme, | |||
}: React.PropsWithChildren<{ | |||
theme?: PartialDeep<Omit<JoyTheme<ExtendedColorScheme>, 'vars'>> & { | |||
theme?: PartialDeep<Omit<JoyTheme, 'vars'>> & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JoyTheme should not be a generic.
@@ -120,6 +87,62 @@ type BaseDesignTokens = { | |||
|
|||
type BaseColorSystem = Pick<BaseDesignTokens, 'palette' | 'shadowRing' | 'shadowChannel'>; | |||
|
|||
const createLightModeVariantVariables = (color: ColorPaletteProp) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no change on these functions, just moved there to prevent dependency cycle.
export interface PalettePrimary extends PaletteRange {} | ||
export interface PaletteNeutral extends PaletteRange {} | ||
export interface PaletteDanger extends PaletteRange {} | ||
export interface PaletteInfo extends PaletteRange {} | ||
export interface PaletteSuccess extends PaletteRange {} | ||
export interface PaletteWarning extends PaletteRange {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split palette to make theme independent (for TS augmentation)
if (!sxObject) { | ||
return null; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the below code caused an error, so I added a condition with a test.
sx={[
theme => theme.typography.something, // undefined
]}
Just to make sure I understand correctly, under the |
Yes, those variants that you specify (higher priority) will be merged with the variants generated from the palette tokens.
It is in const { css: rootCss, vars: rootVars } = cssVarsParser(mergedTheme, {
prefix,
basePrefix: designSystemPrefix,
shouldSkipGeneratingVar,
}); then in line 216, the CSS variables is attach to global stylesheet: <GlobalStyles styles={{ ':root': rootCss }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't spot anything suspicious. I plan to test the joy package, so I am approving assuming this improves the experience. I will report any bugs/problems I will find during the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as Marija! I think I understood the concept, so it's good to go for now. But I'll try it out effectively when playing with building the example UIs :)
Preview https://deploy-preview-30878--material-ui.netlify.app/experiments/joy/variant/
CodeSandbox https://codesandbox.io/s/joy-cra-typescript-forked-h9zus?file=/src/App.tsx
Improve variant customization experience
This PR moves the variant generation from the default theme to run time generation so that developers only need to configure the variables.
Before
Joy defines variant tokens (
theme.palette.primary.*
) and variants (theme.variants.*
) separately. Consequently, if developers want to add/remove some tokens that Joy provides as a default, they will have to adjust thetheme.variants
as well.Take Strapi clone as an example:
Initially, Joy does not define the token
outlinedBg
(the background of the outlined variant).https://www.figma.com/file/9jClRdtL7DgQcG4iYU6MzC/Strapi---UI-Kit-%F0%9F%A7%A9-(Community)?node-id=9974%3A117504
To make Joy looks like Strapi, here is what the developer have to do:
outlinedBg
totheme.palette.primary.outlinedBg
backgroundColor
totheme.variants.outlined
This is just only for primary color, I have to do this for 4 colors. This takes a lot of effort just to customize the background, so it made me think that the process should be easier.
New
I came to realize that the variant tokens in
theme.palette.*
are already explicit.theme.palette.$color.outlinedBg
and you expect the style to work.theme.palette.$color.outlinedHoverBg
and that should be done.The process of building variant styles should be done after Joy has combined the default theme + user-input tokens via
<CssVarsProvider theme={{ colorSchemes: { light: { palette: { ... } }} }}>
.