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.

Sign up for GitHub

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

Jump to bottom

[system] Support variants in theme.styleOverrides #40690

Merged
merged 16 commits into from
Feb 6, 2024
Merged

[system] Support variants in theme.styleOverrides #40690

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
f28ecf3
refactor createStyled to handle variants properly
siriwatknp Jan 19, 2024
efbfadb
fix all bugs
siriwatknp Jan 19, 2024
f605797
fix createStyled
siriwatknp Jan 22, 2024
885d5b2
use deepmerge
siriwatknp Jan 22, 2024
62f04ad
reduce functions to just processStyleArg
siriwatknp Jan 22, 2024
5aa4c8e
restore
siriwatknp Jan 22, 2024
783010d
fix variant callback
siriwatknp Jan 22, 2024
f2285c5
fix types
siriwatknp Jan 22, 2024
3a3a2f8
Merge branch 'master' of https://github.com/mui/material-ui into syst…
siriwatknp Jan 24, 2024
814fbc9
fix theme.variants style callback
siriwatknp Jan 24, 2024
c65c468
Merge branch 'master' of https://github.com/mui/material-ui into syst…
siriwatknp Jan 24, 2024
f69c1f4
fix component overrides type
siriwatknp Jan 24, 2024
473e62d
replace deepmerge with array
siriwatknp Feb 6, 2024
7d7c8a8
fix overrides type
siriwatknp Feb 6, 2024
f79c874
Merge branch 'master' of https://github.com/mui/material-ui into syst…
siriwatknp Feb 6, 2024
9fdd007
fix overrides type to include ownerState
siriwatknp Feb 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/mui-material/src/styles/styled.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,10 @@ describe('styled', () => {
},
});

const testOverridesResolver = (props, styles) => ({
...styles.root,
...(props.variant && styles[props.variant]),
});
const testOverridesResolver = (props, styles) => [
styles.root,
...(props.variant ? [styles[props.variant]] : []),
];
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved

Test = styled('div', {
shouldForwardProp: (prop) => prop !== 'variant' && prop !== 'size' && prop !== 'sx',
Expand Down
261 changes: 71 additions & 190 deletions packages/mui-system/src/createStyled.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
/* eslint-disable no-underscore-dangle */
import styledEngineStyled, { internal_processStyles as processStyles } from '@mui/styled-engine';
import {
getDisplayName,
unstable_capitalize as capitalize,
isPlainObject,
deepmerge,
} from '@mui/utils';
import { getDisplayName, unstable_capitalize as capitalize, isPlainObject } from '@mui/utils';
import createTheme from './createTheme';
import propsToClassKey from './propsToClassKey';
import styleFunctionSx from './styleFunctionSx';

function isEmpty(obj) {
Expand All @@ -25,82 +19,6 @@ function isStringTag(tag) {
);
}

const getStyleOverrides = (name, theme) => {
if (theme.components && theme.components[name] && theme.components[name].styleOverrides) {
return theme.components[name].styleOverrides;
}

return null;
};

const transformVariants = (variants) => {
let numOfCallbacks = 0;
const variantsStyles = {};

if (variants) {
variants.forEach((definition) => {
let key = '';
if (typeof definition.props === 'function') {
key = `callback${numOfCallbacks}`;
numOfCallbacks += 1;
} else {
key = propsToClassKey(definition.props);
}
variantsStyles[key] = definition.style;
});
}

return variantsStyles;
};
const getVariantStyles = (name, theme) => {
let variants = [];
if (theme && theme.components && theme.components[name] && theme.components[name].variants) {
variants = theme.components[name].variants;
}

return transformVariants(variants);
};

const variantsResolver = (props, styles, variants) => {
const { ownerState = {} } = props;
const variantsStyles = [];
let numOfCallbacks = 0;

if (variants) {
variants.forEach((variant) => {
let isMatch = true;
if (typeof variant.props === 'function') {
const propsToCheck = { ...props, ...ownerState };
isMatch = variant.props(propsToCheck);
} else {
Object.keys(variant.props).forEach((key) => {
if (ownerState[key] !== variant.props[key] && props[key] !== variant.props[key]) {
isMatch = false;
}
});
}
if (isMatch) {
if (typeof variant.props === 'function') {
variantsStyles.push(styles[`callback${numOfCallbacks}`]);
} else {
variantsStyles.push(styles[propsToClassKey(variant.props)]);
}
}

if (typeof variant.props === 'function') {
numOfCallbacks += 1;
}
});
}

return variantsStyles;
};

const themeVariantsResolver = (props, styles, theme, name) => {
const themeVariants = theme?.components?.[name]?.variants;
return variantsResolver(props, styles, themeVariants);
};

// Update /system/styled/#api in case if this changes
export function shouldForwardProp(prop) {
return prop !== 'ownerState' && prop !== 'theme' && prop !== 'sx' && prop !== 'as';
Expand All @@ -126,29 +44,32 @@ function defaultOverridesResolver(slot) {
return (props, styles) => styles[slot];
}

const muiStyledFunctionResolver = ({ styledArg, props, defaultTheme, themeId }) => {
const resolvedStyles = styledArg({
...props,
theme: resolveTheme({ ...props, defaultTheme, themeId }),
});

let optionalVariants;
if (resolvedStyles && resolvedStyles.variants) {
optionalVariants = resolvedStyles.variants;
delete resolvedStyles.variants;
}
if (optionalVariants) {
const variantsStyles = variantsResolver(
props,
transformVariants(optionalVariants),
optionalVariants,
);

return [resolvedStyles, ...variantsStyles];
function processStyleArg(styles, props) {
if (typeof styles === 'object' && !!styles) {
const { variants = [], ...otherStyles } = styles;
const variantStyles = [];
variants.forEach((variant) => {
let isMatch = true;
if (typeof variant.props === 'function') {
const propsToCheck = { ...props, ...props.ownerState };
isMatch = variant.props(propsToCheck);
} else {
Object.keys(variant.props).forEach((key) => {
if (props.ownerState?.[key] !== variant.props[key] && props[key] !== variant.props[key]) {
isMatch = false;
}
});
}
if (isMatch) {
variantStyles.push(
typeof variant.style === 'function' ? variant.style(props) : variant.style,
);
}
});
return [otherStyles, ...variantStyles];
}

return resolvedStyles;
};
return styles;
}

export default function createStyled(input = {}) {
const {
Expand Down Expand Up @@ -217,104 +138,64 @@ export default function createStyled(input = {}) {
label,
...options,
});
const muiStyledResolver = (styleArg, ...expressions) => {
const expressionsWithDefaultTheme = expressions
? expressions.map((stylesArg) => {
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
if (typeof stylesArg === 'function' && stylesArg.__emotion_real !== stylesArg) {
return (props) =>
muiStyledFunctionResolver({ styledArg: stylesArg, props, defaultTheme, themeId });
}
if (isPlainObject(stylesArg)) {
let transformedStylesArg = stylesArg;
let styledArgVariants;

if (stylesArg && stylesArg.variants) {
styledArgVariants = stylesArg.variants;
delete transformedStylesArg.variants;

transformedStylesArg = (props) => {
let result = stylesArg;
const variantStyles = variantsResolver(
props,
transformVariants(styledArgVariants),
styledArgVariants,
);
variantStyles.forEach((variantStyle) => {
result = deepmerge(result, variantStyle);
Copy link
Member Author

Choose a reason for hiding this comment

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

I just noticed this deepmerge. I think we should remove it and let emotion/styled-components handle it. Both of them already support array of styles.

@mnajdova Should we try to remove it? the perf should be a bit better while producing the same result.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, would it work if we remove it? From what I remember some of these definitions are merged into one style arg, this is what I remember.

Copy link
Member Author

@siriwatknp siriwatknp Feb 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, it would work but there is a tiny difference in the dev tool

without merging style, you will see all the styles declared in the variants when props are matched:

.hashed-variant2:hover {
  color: red;
}

.hashed-variant1:hover {
  color: green;
}

but the results (cascading) are the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd not merge so that the logic does not depend on the deepmerge but rely on the style engine.

Copy link
Member

Choose a reason for hiding this comment

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

Have we tried removing the deepmerge and see if some test are failing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mnajdova All green, see the change in this 473e62d

});

return result;
};
}
return transformedStylesArg;
}
return stylesArg;
})
: [];

let transformedStyleArg = styleArg;

if (isPlainObject(styleArg)) {
let styledArgVariants;
if (styleArg && styleArg.variants) {
styledArgVariants = styleArg.variants;
delete transformedStyleArg.variants;

transformedStyleArg = (props) => {
let result = styleArg;
const variantStyles = variantsResolver(
props,
transformVariants(styledArgVariants),
styledArgVariants,
);
variantStyles.forEach((variantStyle) => {
result = deepmerge(result, variantStyle);
});

return result;
};
}
} else if (
typeof styleArg === 'function' &&
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
styleArg.__emotion_real !== styleArg
const transformStyleArg = (stylesArg) => {
// On the server Emotion doesn't use React.forwardRef for creating components, so the created
// component stays as a function. This condition makes sure that we do not interpolate functions
// which are basically components used as a selectors.
if (
(typeof stylesArg === 'function' && stylesArg.__emotion_real !== stylesArg) ||
isPlainObject(stylesArg)
) {
// If the type is function, we need to define the default theme.
transformedStyleArg = (props) =>
muiStyledFunctionResolver({ styledArg: styleArg, props, defaultTheme, themeId });
return (props) => {
const resolvedProps = {
...props,
theme: resolveTheme({ theme: props.theme, defaultTheme, themeId }),
};
return processStyleArg(
typeof stylesArg === 'function' ? stylesArg(resolvedProps) : stylesArg,
resolvedProps,
);
};
}
return stylesArg;
};
const muiStyledResolver = (styleArg, ...expressions) => {
let transformedStyleArg = transformStyleArg(styleArg);
const expressionsWithDefaultTheme = expressions ? expressions.map(transformStyleArg) : [];

if (componentName && overridesResolver) {
expressionsWithDefaultTheme.push((props) => {
const theme = resolveTheme({ ...props, defaultTheme, themeId });
const styleOverrides = getStyleOverrides(componentName, theme);
Copy link
Member Author

@siriwatknp siriwatknp Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

getStyleOverrides is not reused, so remove it to not shift between functions.


if (styleOverrides) {
const resolvedStyleOverrides = {};
Object.entries(styleOverrides).forEach(([slotKey, slotStyle]) => {
resolvedStyleOverrides[slotKey] =
typeof slotStyle === 'function' ? slotStyle({ ...props, theme }) : slotStyle;
});
return overridesResolver(props, resolvedStyleOverrides);
if (
!theme.components ||
!theme.components[componentName] ||
!theme.components[componentName].styleOverrides
) {
return null;
}

return null;
const styleOverrides = theme.components[componentName].styleOverrides;
const resolvedStyleOverrides = {};
const resolvedProps = { ...props, theme };
Object.entries(styleOverrides).forEach(([slotKey, slotStyle]) => {
resolvedStyleOverrides[slotKey] = processStyleArg(
typeof slotStyle === 'function' ? slotStyle(resolvedProps) : slotStyle,
resolvedProps,
);
});
return overridesResolver(props, resolvedStyleOverrides);
});
}

if (componentName && !skipVariantsResolver) {
expressionsWithDefaultTheme.push((props) => {
const theme = resolveTheme({ ...props, defaultTheme, themeId });
return themeVariantsResolver(
props,
getVariantStyles(componentName, theme),
theme,
componentName,
const themeVariants = theme?.components?.[componentName]?.variants;
return processStyleArg(
{ variants: themeVariants },
{
...props,
theme: resolveTheme({ theme, defaultTheme, themeId }),
},
);
});
}
Expand Down
55 changes: 51 additions & 4 deletions packages/mui-system/src/styled.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,31 @@ describe('styled', () => {
height: '400px',
},
},
{
props: ({ variant }) => variant === 'circle',
style: ({ theme: t }) => ({
width: t.shape.borderRadius,
height: t.shape.borderRadius,
}),
},
],
styleOverrides: {
root: {
width: '250px',
variants: [
{
props: { variant: 'triangle' },
style: {
width: '250px',
},
},
{
props: ({ variant }) => variant === 'triangle',
style: ({ theme: t }) => ({
height: t.shape.borderRadius,
}),
},
],
},
rect: {
height: '250px',
Expand All @@ -172,10 +193,10 @@ describe('styled', () => {
},
});

const testOverridesResolver = (props, styles) => ({
...styles.root,
...(props.variant && styles[props.variant]),
});
const testOverridesResolver = (props, styles) => [
styles.root,
...(props.variant ? [styles[props.variant]] : []),
];

Test = styled('div', {
shouldForwardProp: (prop) => prop !== 'variant' && prop !== 'size' && prop !== 'sx',
Expand Down Expand Up @@ -394,6 +415,32 @@ describe('styled', () => {
});
});

it('variants should work with callbacks', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<Test variant="circle">Test</Test>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: `${theme.shape.borderRadius}px`,
height: `${theme.shape.borderRadius}px`,
});
});

it('variants should work inside overrides', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<Test variant="triangle">Test</Test>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '250px',
height: `${theme.shape.borderRadius}px`,
});
});

it('styled wrapper should win over variants', () => {
const CustomTest = styled(Test)`
width: 500px;
Expand Down
Loading