-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[experimentalStyled] Make sx style fn optional #23714
[experimentalStyled] Make sx style fn optional #23714
Conversation
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.
@mnajdova Interesting approach. Thoughts:
- I think that the option to disable the
sx
prop is interesting for design teams that want to minimize entropy. It's not unusual to have developers that want to enforce high constraints on the exposed API. I'm not sure, however, that it's common enough yet to support it, considering a workaround is to wrap. Maybe it would even work better with an option in the theme. - Instead of disabling the
sx
prop, what would be wrong if a developer uses a different name for the prop? - In [system] transform and themeKey do not work together when using style from material-ui/system #23496, the root of the issue for the author seems to be a missing
spacing
feature. Something developer can find alternatives in: tailwindcss, or all the Stack components. I start to wonder if we even need a Stack component if aspacingX
andspacingY
value in the system wouldn't be enough. - In [system] transform and themeKey do not work together when using style from material-ui/system #23496, the problem I was wondering about is the notion of a custom superset of CSS you can use. It might be overkill to have it in the theme as done in https://stitches.dev/docs/utils, considering we didn't yet see a developer that had this use case.
if (!skipSx) { | ||
expressionsWithDefaultTheme.push((props) => { | ||
const theme = isEmpty(props.theme) ? defaultTheme : props.theme; | ||
return styleFunctionSx({ ...props, theme }); |
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 might understand why the curried API on styled-system, css() method, maybe it's so they don't need to spread the props, I wonder if it helps with performance:
return styleFunctionSx({ ...props, theme }); | |
return styleFunctionSx(props)(theme); |
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.
Let me test this separately, wouldn't like to break something else somewhere :)))
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.
But looks like it really may help
The biggest problem and the reason why I decided to add this option is the following -
our default
Maybe adding a
At this moment I wouldn't go that far. Adding custom props and handling of existing ones is pretty easy anyway as demonstrated on the codesandbox - https://codesandbox.io/s/confident-bush-346hb?file=/src/App.js I would stay away from introducing new API until we really see a value in it, otherwise, we will again introduce new abstractions that developers will need to learn... |
This PR adds an option for making the
sx
prop optional as part of theexperimentalStyled
utility. This could unblock developers to customize the behavior of the resolver for thesx
prop, depending on their needs. A simple example of how it could be used can be found here - https://codesandbox.io/s/confident-bush-346hb?file=/src/App.js (the value for them
prop is always the negative value of the resolved one).Discussion around this issue can be found here - #23496