-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Decouple sx
prop from system and add docs
#57
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.
The shorthand properties (currently, the shorthand works but it's tied to MUI System styleFunctionSx). There is a plan to move to theme.unstable_sx so that developer can configure their own which will be a different PR.
This is very important, I would even wait with adding documentation until we have this. The sx prop currently is tight to a theme structure, that people may not be aware of. If we don't have a documentation about this, it looks like magic.
README.md
Outdated
- a callback function that receives the [theme object](#theming) then return a plain style object: | ||
|
||
```js | ||
<div sx={(theme) => ({ color: theme.colors.primary })} /> |
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.
Maybe we can add example that is not possible with an object, so that people are aware of what's the different use-case.
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.
What do you mean by "not possible with an object"? Can you give me an example?
README.md
Outdated
- an array of plain style objects or callback functions. This is useful for applying conditional styles based on other variables: | ||
|
||
```js | ||
<div |
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.
Great example!
README.md
Outdated
|
||
The value provided to `sx` prop can be one of the following: | ||
|
||
- a plain style object (recommended) |
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'd also mention static values.
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.
Can you give me an example?
sx
prop to readme (part 1)sx
prop to readme
sx
prop from systemsx
prop from system and add docs
interface HTMLAttributes<T> { | ||
sx?: | ||
| React.CSSProperties | ||
| ((theme: Theme) => React.CSSProperties) |
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.
We should also add where to import this Theme
from.
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.
Added as a placeholder to imply that it comes from the consumer.
I decided to change the intention of this PR so that we can ship incrementally.
From Pigment CSS users,
sx
prop gives a convenient way to style elements without writing any import. The shorthand should not be enabled by default (it's too magical).If they want shorthand, they need to add some theme config (a new feature for Pigment CSS).
So this PR is about decoupling the shorthand from System so that
sx
prop does not come with shorthand by default.Then consider shorthand as another feature that can be worked separately.