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
Changes from 22 commits
d416262
a4ac85f
9d0d4f3
7e1815a
4a6bafb
1e556dc
bf55df5
2f2c1a3
696baf0
150fab6
2c1c0e1
aa8dab7
5593dc9
db412b5
c9b5e14
3c27f52
c71e273
457d61d
6205b27
2a282d1
aab6ec2
5556acb
d79f580
e23405b
d6756ea
3d13353
1bb3f11
0d71255
a41f4b0
1d116d1
0393080
bcf632c
215a70e
54489dc
6109c6d
eb4e6cb
a719745
d55b720
8f4a288
95fc5e1
b7e3a5a
ab6b951
15c3668
985012e
ab13d5b
2cbf11c
3b3f5b1
a387ed7
e5039b0
21f55ab
cf584ee
4cf4ce2
9afb562
0debf67
d68bbe6
80a221f
c0ffaba
e42d74c
75b5374
37c6b59
99a7021
743c1c8
4a5ecc5
ad8eb6d
ff505be
cdd96c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'; | ||
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I would be nice if the const Button = styled('button')(({ theme }) => theme.unstable_sx({
px: 1,
// ...
})) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great point, yes we can do this. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Is the debug experience for developers a problem with the current
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} />
I think that it's equivalent to: const Button = styled('button')(
- experimental_sx({ ... }),
-)
+ { ... }
+, { unstable_sxSyntax: true }); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 With your proposal, logic lives inside the With There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not for the case that simple styles are mixed with const Button = styled('button')(({ theme }) => ([
{ padding: 8 },
experimental_sx({
'&:hover': {
bgcolor: 'primary.main',
}
})
])) Adding There was a problem hiding this comment. Choose a reason for hiding this commentThe 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',
},
})
]));
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
How would it help when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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 |
||
export { ColorInversionProvider, useColorInversion } from './ColorInversion'; | ||
export { default as extendTheme, createGetCssVar } from './extendTheme'; | ||
export type { CssVarsThemeOptions } from './extendTheme'; | ||
|
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.
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.
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.
Honestly, I would be against adding tests for internals (as that is what
styleFunctionSx
is, and rather relay on thesx
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)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.
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.
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.
Agree 👍