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

[system] Enable configuring the sx prop in the theme #35150

Merged
merged 66 commits into from Dec 7, 2022
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
66 commits
Select commit Hold shift + click to select a range
d416262
[release] v5.10.12
mnajdova Oct 31, 2022
a4ac85f
Remove autogenerated text
mnajdova Oct 31, 2022
9d0d4f3
Merge branch 'master' into release/5.10.12
mnajdova Oct 31, 2022
7e1815a
Updated changelog
mnajdova Oct 31, 2022
4a6bafb
Merge branch 'master' into release/5.10.12
mnajdova Oct 31, 2022
1e556dc
Updated changelog
mnajdova Oct 31, 2022
bf55df5
Merge branch 'release/5.10.12' of https://github.com/mnajdova/materia…
mnajdova Oct 31, 2022
2f2c1a3
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 4, 2022
696baf0
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 9, 2022
150fab6
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 14, 2022
2c1c0e1
[system] Explore different sx config
mnajdova Nov 14, 2022
aa8dab7
lint issues
mnajdova Nov 15, 2022
5593dc9
Refactor Joy's usage
mnajdova Nov 15, 2022
db412b5
Fix experimental sx
mnajdova Nov 15, 2022
c9b5e14
Fixes
mnajdova Nov 15, 2022
3c27f52
More fixes
mnajdova Nov 15, 2022
c71e273
Moved and fixed some tests
mnajdova Nov 15, 2022
457d61d
fixes & lint issues
mnajdova Nov 15, 2022
6205b27
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 15, 2022
2a282d1
Merge branch 'master' into chore/sx-improvements
mnajdova Nov 15, 2022
aab6ec2
Update packages/mui-joy/src/Box/Box.test.js
mnajdova Nov 16, 2022
5556acb
Update packages/mui-joy/src/styles/defaultTheme.ts
mnajdova Nov 16, 2022
d79f580
Resolve comments from review
mnajdova Nov 17, 2022
e23405b
Resolve comments from review
mnajdova Nov 17, 2022
d6756ea
Resolve review comments
mnajdova Nov 18, 2022
3d13353
prettier
mnajdova Nov 18, 2022
1bb3f11
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 18, 2022
0d71255
Update docs' system performance section
mnajdova Nov 21, 2022
a41f4b0
Convert to TS
mnajdova Nov 21, 2022
1d116d1
Use declaration file
mnajdova Nov 21, 2022
0393080
lint
mnajdova Nov 21, 2022
bcf632c
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 21, 2022
215a70e
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 21, 2022
54489dc
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 22, 2022
6109c6d
Merge branch 'master' into chore/sx-improvements
mnajdova Nov 22, 2022
eb4e6cb
Update @mui/material-next witht he latest sx config
mnajdova Nov 22, 2022
a719745
TS fixes
mnajdova Nov 22, 2022
d55b720
lint fixes
mnajdova Nov 22, 2022
8f4a288
lint
mnajdova Nov 22, 2022
95fc5e1
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 23, 2022
b7e3a5a
Make sx work for both MD2 and MD3
mnajdova Nov 25, 2022
ab6b951
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 28, 2022
15c3668
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Nov 28, 2022
985012e
Update packages/mui-material-next/src/styles/extendTheme.ts
mnajdova Nov 29, 2022
ab13d5b
Add more tests
mnajdova Nov 29, 2022
2cbf11c
Merge branch 'chore/sx-improvements' of https://github.com/mnajdova/m…
mnajdova Nov 29, 2022
3b3f5b1
More generic solution to the `materialYouComponent` flag
mnajdova Nov 29, 2022
a387ed7
Resolve review comments
mnajdova Nov 30, 2022
e5039b0
lint
mnajdova Nov 30, 2022
21f55ab
Remove `experimental_sx`
mnajdova Nov 30, 2022
cf584ee
Small fixes & docs updates
mnajdova Nov 30, 2022
4cf4ce2
Fix test & translations
mnajdova Nov 30, 2022
9afb562
Apply suggestions from code review
mnajdova Dec 1, 2022
0debf67
Update packages/mui-joy/src/styles/CssVarsProvider.tsx
mnajdova Dec 1, 2022
d68bbe6
Update packages/mui-system/src/styleFunctionSx/styleFunctionSx.js
mnajdova Dec 1, 2022
80a221f
Update docs/data/system/experimental-api/extend-sx-prop/ExtendTheSxPr…
mnajdova Dec 1, 2022
c0ffaba
Update docs/data/system/experimental-api/extend-sx-prop/ChangeTheBeha…
mnajdova Dec 1, 2022
e42d74c
Update docs/data/system/experimental-api/extend-sx-prop/ChangeTheBeha…
mnajdova Dec 1, 2022
75b5374
Address review comments
mnajdova Dec 1, 2022
37c6b59
lint issues
mnajdova Dec 1, 2022
99a7021
types
mnajdova Dec 1, 2022
743c1c8
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Dec 6, 2022
4a5ecc5
Update docs/data/system/experimental-api/configure-the-sx-prop/config…
mnajdova Dec 7, 2022
ad8eb6d
Merge branch 'master' of https://github.com/mui-org/material-ui into …
mnajdova Dec 7, 2022
ff505be
Merge branch 'master' into chore/sx-improvements
mnajdova Dec 7, 2022
cdd96c9
prettier
mnajdova Dec 7, 2022
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
187 changes: 186 additions & 1 deletion packages/mui-joy/src/Box/Box.test.js
@@ -1,7 +1,7 @@
import * as React from 'react';
import { expect } from 'chai';
import { createRenderer, describeConformance } from 'test/utils';
import { ThemeProvider } from '@mui/joy/styles';
import { ThemeProvider, CssVarsProvider, extendTheme } from '@mui/joy/styles';
import { unstable_ClassNameGenerator as ClassNameGenerator } from '@mui/joy/className';
import Box from '@mui/joy/Box';

Expand Down Expand Up @@ -65,4 +65,189 @@ describe('Joy <Box />', () => {
expect(container.firstChild).to.have.class('CompanyBox-root');
});
});

describe('sx', () => {
const theme = extendTheme({
colorSchemes: {
light: {
palette: {
primary: {
500: 'rgb(0, 0, 255)',
},
},
},
},
radius: {
md: '77px',
},
shadow: {
md: 'rgb(0, 0, 0) 0px 0px 10px 0px',
},
});

it('color', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ color: 'primary.500' }} />
</CssVarsProvider>,
);

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

it('bgcolor', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ bgcolor: 'primary.500' }} />
</CssVarsProvider>,
);

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

it('backgroundColor', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ backgroundColor: 'primary.500' }} />
</CssVarsProvider>,
);

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

it('borderRadius', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ borderRadius: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
borderTopLeftRadius: '77px',
borderTopRightRadius: '77px',
borderBottomLeftRadius: '77px',
borderBottomRightRadius: '77px',
});
});

it('boxShadow', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ boxShadow: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
boxShadow: 'rgb(0, 0, 0) 0px 0px 10px 0px',
});
});

it('fontSize', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ fontSize: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
fontSize: '16px',
});
});

it('fontWeight', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ fontWeight: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
fontWeight: '500',
});
});

it('letterSpacing', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ letterSpacing: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
letterSpacing: '1.328px',
});
});

it('lineHeight', function test() {
const isJSDOM = /jsdom/.test(window.navigator.userAgent);

if (isJSDOM) {
this.skip();
}

const { container } = render(
<CssVarsProvider theme={theme}>
<Box sx={{ lineHeight: 'md' }} />
</CssVarsProvider>,
);

expect(container.firstChild).toHaveComputedStyle({
lineHeight: '24px',
});
});
Comment on lines +235 to +251
Copy link
Member

Choose a reason for hiding this comment

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

Since we have added quite a number of these tests, I wonder if there is an opportunity with lower-level tests. I'm not sure, only that not having JSDOM can make iterating on tests a bit slower.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I would be against adding tests for internals (as that is what styleFunctionSx is, and rather relay on the sx prop to work as expected, as that is the goal). As we've seen now, changing the internals required writing different tests, which is not great. So, having this said, I would ultimately want to avoid it next time we need to do some internal changes (maybe update the system to use utility classes :D)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I guess that it would be relevant once we make the sx function transformation in the public API, either through theme.sx or with an import.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree 👍

});
});
2 changes: 0 additions & 2 deletions packages/mui-joy/src/Box/Box.tsx
Expand Up @@ -4,13 +4,11 @@ import { OverridableComponent } from '@mui/types';
import { unstable_ClassNameGenerator as ClassNameGenerator } from '../className';
import { BoxTypeMap } from './BoxProps';
import defaultTheme from '../styles/defaultTheme';
import styleFunctionSx from '../styles/styleFunctionSx';

const Box = createBox({
defaultTheme,
defaultClassName: 'JoyBox-root',
generateClassName: ClassNameGenerator.generate,
styleFunctionSx,
}) as OverridableComponent<BoxTypeMap>;

Box.propTypes /* remove-proptypes */ = {
Expand Down
1 change: 1 addition & 0 deletions packages/mui-joy/src/styles/defaultTheme.test.js
Expand Up @@ -29,6 +29,7 @@ describe('defaultTheme', () => {
'vars',
'cssVarPrefix',
'getColorSchemeSelector',
'sxConfig',
]).to.includes(field);
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/mui-joy/src/styles/extendTheme.test.js
Expand Up @@ -23,6 +23,7 @@ describe('extendTheme', () => {
'colorInversionConfig',
'variants',
'cssVarPrefix',
'sxConfig',
]).to.includes(field);
});
});
Expand Down
7 changes: 7 additions & 0 deletions packages/mui-joy/src/styles/extendTheme.ts
Expand Up @@ -7,6 +7,7 @@ import {
unstable_createGetCssVar as systemCreateGetCssVar,
colorChannel,
} from '@mui/system';
import defaultSxConfig from './sxConfig';
import colors from '../colors';
import { DefaultColorScheme, ExtendedColorScheme } from './types/colorScheme';
import { ColorSystem, ColorPaletteProp, PaletteRange } from './types/colorSystem';
Expand Down Expand Up @@ -60,6 +61,7 @@ export interface CssVarsThemeOptions extends Partial2Level<ThemeScales> {
spacing?: SpacingOptions;
components?: Components<Theme>;
colorSchemes?: Partial<Record<DefaultColorScheme | ExtendedColorScheme, ColorSystemOptions>>;
sxConfig?: object;
}

export const createGetCssVar = (cssVarPrefix = 'joy') =>
Expand Down Expand Up @@ -623,5 +625,10 @@ export default function extendTheme(themeOptions?: CssVarsThemeOptions): Theme {
attachColorChannels(colorSystem.palette);
});

theme.sxConfig = {
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
...defaultSxConfig,
...themeOptions?.sxConfig,
};

return theme;
}
2 changes: 1 addition & 1 deletion packages/mui-joy/src/styles/index.ts
@@ -1,5 +1,6 @@
// reexports from system for module augmentation
export type { BreakpointOverrides } from '@mui/system';
export { experimental_sx } from '@mui/system';

// Joy typings
export type { ColorSchemeOverrides, SupportedColorScheme } from './types/colorScheme';
Expand Down Expand Up @@ -71,7 +72,6 @@ export { default as styled } from './styled';
export { default as ThemeProvider } from './ThemeProvider';
export * from './ThemeProvider';
export { default as useThemeProps } from './useThemeProps';
export { sx as experimental_sx } from './styleFunctionSx';
Copy link
Member

Choose a reason for hiding this comment

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

What would happen to Material UI? Do you think we should drop the experimental_sx?

I would be nice if the sx is attached to the theme directly like what I proposed in https://github.com/mui/material-ui/pull/35081/files:

const Button = styled('button')(({ theme }) => theme.unstable_sx({
  px: 1,
  // ...
}))

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point, yes we can do this.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if an API close to this one would be a better DX:

const Button = styled('button')({
  px: 1,
  // ...
}), { unstable_sxSyntax: true });

Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari the downside of that approach is that it is hard to debug.

The theme.unstable_sx(...) is more flexible:

  • You can debug the result before it goes to emotion
    const Button = styled('button')(({ theme }) => {
      const result = theme.unstable_sx({
        px: 1,
        // ...
      });
      return result;
    })
  • Since it attaches to the theme, you can use it anywhere
    const theme = useTheme();
    const style = theme.unstable_sx({ ... });
    
    <div style={style} />
  • Migration from experimental_sx looks easier:
    const Button = styled('button')(
    -  experimental_sx({ ... }),
    + ({ theme }) => theme.unstable_sx({ ... })
    )

Copy link
Member

Choose a reason for hiding this comment

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

@siriwatknp From how I read this, the root of the question is: Do we want to trade less tree-shaking capabilities (see the bundle size increase) in exchange for a transformation utils easier to access (instead of an import, right from the theme). Personally, I don't see a clear win but no objections to moving forward as is.

the downside of that approach is that it is hard to debug.

Is the debug experience for developers a problem with the current sx prop?

Since it attaches to the theme, you can use it anywhere

What's the use case to use the function with inline style? Or, why is the current API not good enough?

import { styleFunctionSx } from '@mui/system';

const theme = useTheme();
const style = styleFunctionSx({ ..., theme });

<div style={style} />

Migration from experimental_sx looks easier

I think that it's equivalent to:

const Button = styled('button')(
-  experimental_sx({ ... }),
-)
+  { ... }
+, { unstable_sxSyntax: true });

Copy link
Member

Choose a reason for hiding this comment

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

Is the debug experience for developers a problem with the current sx prop?

Definitely yes. It takes me a while to figure out why the width is '100%':

<Box sx={{ width: 1 }} /> // I want to create a divider.

The DX here is bad because I don't know what I did wrong. However, I am not talking about sx here. What I mentioned was about comparing theme.unstable_sx and { unstable_sxSyntax: true }.

With your proposal, logic lives inside the styled which takes time for developers to figure out what's wrong with their code. I think the whole purpose of this PR is to move the logic to the theme to make it more flexible.

With theme.unstable_sx, it is just a function and more flexible. I can debug the result of it then I will see that this function somehow turns the width: 100%. The feedback loop is very short.

Copy link
Member

Choose a reason for hiding this comment

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

I think that it's equivalent to:

Not for the case that simple styles are mixed with experimental_sx:

const Button = styled('button')(({ theme }) => ([
  { padding: 8 },
  experimental_sx({
    '&:hover': {
      bgcolor: 'primary.main',
    }
  })
]))

Adding { unstable_sxSyntax: true } will turn padding: 8 to padding theme.spacing(8).

Copy link
Member

Choose a reason for hiding this comment

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

@siriwatknp Right, ok.

Then I think that it could be valuable to consider this alternative API. Maybe it can help with tree shaking and doesn't harm the DX too much:

import { styled, unstable_sx } from '@mui/system';

const Button = styled('button')(({ theme }) => ([
  { padding: 8 },
  unstable_sx(theme, {
    '&:hover': {
      bgcolor: 'primary.main',
    },
  })
]));

It takes me a while to figure out why the width is '100%':

I think that it could be great to make a breaking change, to remove 1 === 100%. I had the same frustration in the past. +1 to open a GitHub issue about this one.

Copy link
Member

Choose a reason for hiding this comment

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

tree shaking

How would it help when unstable_sx is used internally by the sx prop?

Copy link
Member

Choose a reason for hiding this comment

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

How would it help when unstable_sx is used internally by the sx prop?

@siriwatknp https://mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=454127&baseRef=master&baseCommit=b8b1e0d08ceab826a3e11b18a3b4b6e6b5c17206&prNumber=35150 answers this question. There are components that imports the theme but not the styled function, they are the ones that takes a +2kB gzipped hit.

But I agree that to fully benefit from this, we would need to change the default option of createStyled in the system to not come with the sx helpers by default. Not sure if it's better.

export { ColorInversionProvider, useColorInversion } from './ColorInversion';
export { default as extendTheme, createGetCssVar } from './extendTheme';
export type { CssVarsThemeOptions } from './extendTheme';
Expand Down