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

[styled-engine] Fix tagged template syntax with multiple expressions #23269

Merged
merged 8 commits into from Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 2 additions & 9 deletions packages/material-ui-styled-engine-sc/src/index.js
@@ -1,21 +1,14 @@
import scStyled from 'styled-components';

export default function styled(tag, options) {
let scStyledPrepared;

if (options) {
scStyledPrepared = scStyled(tag).withConfig({
return scStyled(tag).withConfig({
displayName: options.label,
shouldForwardProp: options.shouldForwardProp,
});
} else {
scStyledPrepared = scStyled(tag);
}

// TODO: This should not be required once we solve the warning `You have illegal escape sequence in your template literal`
return (styles) => {
return scStyledPrepared(...styles);
};
return scStyled(tag);
}

export { ThemeContext } from 'styled-components';
54 changes: 37 additions & 17 deletions packages/material-ui/src/styles/experimentalStyled.js
Expand Up @@ -64,34 +64,54 @@ const shouldForwardProp = (prop) => prop !== 'styleProps' && prop !== 'theme' &&
const experimentalStyled = (tag, options, muiOptions = {}) => {
const name = muiOptions.muiName;
const defaultStyledResolver = styled(tag, { shouldForwardProp, label: name, ...options });
const muiStyledResolver = (...styles) => {
const stylesWithDefaultTheme = styles.map((stylesArg) => {
return typeof stylesArg === 'function'
? ({ theme: themeInput, ...rest }) =>
stylesArg({ theme: isEmpty(themeInput) ? defaultTheme : themeInput, ...rest })
: stylesArg;
});
const muiStyledResolver = (styleArg, ...expressions) => {
const expressionsWithDefaultTheme = expressions
? expressions.map((stylesArg) => {
return typeof stylesArg === 'function'
? ({ theme: themeInput, ...rest }) => {
return stylesArg({
theme: isEmpty(themeInput) ? defaultTheme : themeInput,
...rest,
});
}
: stylesArg;
})
: [];

let transformedStyleArg = styleArg;

if (Array.isArray(styleArg)) {
// If the type is array, than we need to add placeholders in the template for the overrides, variants and the sx styles
transformedStyleArg = [...styleArg, '', '', ''];
Copy link
Member Author

Choose a reason for hiding this comment

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

This was my main point of interest since transpiling this results in quite a bit of babel helpers if array-spread isn't supported. But that's only the case for our /legacy bundle in v5 🎉

Though my the micro-perf part of my brain does want to dive into some v8 internals and see what's better 😁

-[...styleArg, '']
+styleArg.concat('')

Copy link
Member

Choose a reason for hiding this comment

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

In my mind, when I see

[...styleArg, '']

I am able to immediately conclude that this is a copy of the array with additional elements, for every other syntax I would need to take a closer look to decide whether it's a copy or mutation :) Of course this is probably just my preference, but you are probably right that it requires more advanced transpiling behind the scene :D

transformedStyleArg.raw = [...styleArg.raw, '', '', ''];
} else if (typeof styleArg === 'function') {
// If the type is function, we need to define the default theme
transformedStyleArg = ({ theme: themeInput, ...rest }) =>
styleArg({ theme: isEmpty(themeInput) ? defaultTheme : themeInput, ...rest });
}

if (name && muiOptions.overridesResolver) {
stylesWithDefaultTheme.push((props) => {
expressionsWithDefaultTheme.push((props) => {
if (name && muiOptions.overridesResolver) {
const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
return muiOptions.overridesResolver(props, getStyleOverrides(name, theme), name);
});
}
}
return '';
});

if (name) {
stylesWithDefaultTheme.push((props) => {
expressionsWithDefaultTheme.push((props) => {
if (name) {
const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
return variantsResolver(props, getVariantStyles(name, theme), theme, name);
});
}
}
return '';
});

stylesWithDefaultTheme.push((props) => {
expressionsWithDefaultTheme.push((props) => {
const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
return styleFunctionSx(props.sx, theme);
});

return defaultStyledResolver(stylesWithDefaultTheme);
return defaultStyledResolver(transformedStyleArg, ...expressionsWithDefaultTheme);
};
return muiStyledResolver;
};
Expand Down
168 changes: 167 additions & 1 deletion packages/material-ui/src/styles/experimentalStyled.test.js
@@ -1,6 +1,6 @@
import React from 'react';
import { expect } from 'chai';
import { createClientRender } from 'test/utils';
import { createClientRender, screen } from 'test/utils';
import createMuiTheme from './createMuiTheme';
import styled from './experimentalStyled';
import ThemeProvider from './ThemeProvider';
Expand All @@ -9,6 +9,18 @@ describe('experimentalStyled', () => {
const render = createClientRender();

it('should work', () => {
const Div = styled('div')`
width: 200px;
`;

const { container } = render(<Div>Test</Div>);

expect(container.firstChild).toHaveComputedStyle({
width: '200px',
});
});

it('should work when styles are object', () => {
const Div = styled('div')({
width: '200px',
});
Expand All @@ -21,6 +33,18 @@ describe('experimentalStyled', () => {
});

it('should use defaultTheme if no theme is provided', () => {
const Div = styled('div')`
width: ${(props) => props.theme.spacing(1)};
`;

const { container } = render(<Div>Test</Div>);

expect(container.firstChild).toHaveComputedStyle({
width: '8px',
});
});

it('should use defaultTheme if no theme is provided when styles are object', () => {
const Div = styled('div')((props) => ({
width: props.theme.spacing(1),
}));
Expand All @@ -33,6 +57,26 @@ describe('experimentalStyled', () => {
});

it('should use theme from context if available', () => {
const Div = styled('div')`
width: ${(props) => props.theme.spacing(1)};
`;

const theme = createMuiTheme({
spacing: 10,
});

const { container } = render(
<ThemeProvider theme={theme}>
<Div>Test</Div>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '10px',
});
});

it('should use theme from context if available when styles are object', () => {
const Div = styled('div')((props) => ({
width: props.theme.spacing(1),
}));
Expand All @@ -52,6 +96,34 @@ describe('experimentalStyled', () => {
});
});

describe('dynamic styles', () => {
it('can adapt styles to props', () => {
const Div = styled('div')`
font-size: ${(props) => props.scale * 8}px;
padding-left: ${(props) => props.scale * 2}px;
`;
render(<Div scale={4} data-testid="target" />);

expect(screen.getByTestId('target')).toHaveComputedStyle({
fontSize: '32px',
paddingLeft: '8px',
});
});

it('can adapt styles to props when styles are object', () => {
const DivObj = styled('div')((props) => ({
fontSize: `${props.scale * 8}px`,
paddingLeft: `${props.scale * 2}px`,
}));
render(<DivObj scale={4} data-testid="target" />);

expect(screen.getByTestId('target')).toHaveComputedStyle({
fontSize: '32px',
paddingLeft: '8px',
});
});
});

describe('muiOptions', () => {
/**
* @type {ReturnType<typeof createMuiTheme>}
Expand All @@ -61,6 +133,10 @@ describe('experimentalStyled', () => {
* @type {ReturnType<typeof styled>}
*/
let Test;
/**
* @type {ReturnType<typeof styled>}
*/
let TestObj;

before(() => {
theme = createMuiTheme({
Expand Down Expand Up @@ -105,6 +181,15 @@ describe('experimentalStyled', () => {
width: 200px;
height: 300px;
`;

TestObj = styled(
'div',
{ shouldForwardProp: (prop) => prop !== 'variant' && prop !== 'size' && prop !== 'sx' },
{ muiName: 'MuiTest', overridesResolver: testOverridesResolver },
)({
width: '200px',
height: '300px',
});
});

it('should work with specified muiOptions', () => {
Expand All @@ -116,6 +201,15 @@ describe('experimentalStyled', () => {
});
});

it('should work with specified muiOptions when styles are object', () => {
const { container } = render(<TestObj>Test</TestObj>);

expect(container.firstChild).toHaveComputedStyle({
width: '200px',
height: '300px',
});
});

it('overrides should be respected', () => {
const { container } = render(
<ThemeProvider theme={theme}>
Expand All @@ -129,6 +223,19 @@ describe('experimentalStyled', () => {
});
});

it('overrides should be respected when styles are object', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<TestObj>Test</TestObj>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '250px',
height: '300px',
});
});

it('overrides should be respected when prop is specified', () => {
const { container } = render(
<ThemeProvider theme={theme}>
Expand All @@ -142,6 +249,19 @@ describe('experimentalStyled', () => {
});
});

it('overrides should be respected when prop is specified when styles are object', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<TestObj variant="rect">Test</TestObj>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '250px',
height: '250px',
});
});

it('variants should win over overrides', () => {
const { container } = render(
<ThemeProvider theme={theme}>
Expand All @@ -157,6 +277,21 @@ describe('experimentalStyled', () => {
});
});

it('variants should win over overrides when styles are object', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<TestObj variant="rect" size="large">
Test
</TestObj>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '400px',
height: '400px',
});
});

it('styled wrapper should win over variants', () => {
const CustomTest = styled(Test)`
width: 500px;
Expand All @@ -176,6 +311,25 @@ describe('experimentalStyled', () => {
});
});

it('styled wrapper should win over variants when styles are object', () => {
const CustomTest = styled(TestObj)({
width: '500px',
});

const { container } = render(
<ThemeProvider theme={theme}>
<CustomTest variant="rect" size="large">
Test
</CustomTest>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
width: '500px',
height: '400px',
});
});

it('should resolve the sx prop', () => {
const { container } = render(
<ThemeProvider theme={theme}>
Expand All @@ -187,5 +341,17 @@ describe('experimentalStyled', () => {
color: 'rgb(0, 0, 255)',
});
});

it('should resolve the sx prop when styles are object', () => {
const { container } = render(
<ThemeProvider theme={theme}>
<TestObj sx={{ color: 'primary.main' }}>Test</TestObj>
</ThemeProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
color: 'rgb(0, 0, 255)',
});
});
});
});