[Slider] Add system prop #22819
[Slider] Add system prop #22819
Conversation
|
@material-ui/core: parsed: +0.07% , gzip: +0.11% |
| /** | ||
| * Common system props. | ||
| */ | ||
| system: PropTypes.object, |
There was a problem hiding this comment.
We should probably come up with better name here... But I would definitely vote for defining all system props under one prop instead of spreading them on each component, mainly because of the following reasons:
- we are already specifying all styles props under one prop -
stylesProps - we would avoid props explosion
- all system props would be easily discoverable
There was a problem hiding this comment.
I was thinking more of a prop like styles, or css (css will conflict with emotion's css), but even systemProps sounds better :)
There was a problem hiding this comment.
Let's get some other feedback before changing it.
There was a problem hiding this comment.
Should we have a 📊 on Twitter about the best API for the system? styled-system pushes for flattening; theme-ui.com pushes for isolation under one prop, well they have props too. Both have the same author.
A benchmark: https://trello.com/c/IUve1J6o/1577-system-core-component
There was a problem hiding this comment.
and well known
We need to be careful with that statement and consider audiences. A very large portion have a backend background. So far I've only heard of it in the context of Theme-UI.
There was a problem hiding this comment.
Regarding the flatten vs prop tradeoff. Any thoughts on it? The summarized version:
- Box component: subset of CSS, flattened system props + superset of CSS,
systemprop - Most core components: superset of CSS,
systemprop only - Some core components that are responsible for accessing design tokens or doing layout have flattened system prop: Grid, Stack, Typography (e.g. fontWeight).
There was a problem hiding this comment.
So system > sx > css?
Yep, and I am saying system in case if we are going to use it as a simply collection of system props, if we are going to treat is as a superset of CSS, then I would rather use a prop like styles or sx for it... In my opinion there is a big difference between these two use-cases
Regarding the flatten vs prop tradeoff. Any thoughts on it? The summarized version:
Box component: subset of CSS, flattened system props + superset of CSS, system prop
Agree, again if we are using the system props as superset of CSS I am revoking the system prop as a name, as it is not system anymore is more styles/css. 😄
Most core components: superset of CSS, system prop only
Agree 👍
Some core components that are responsible for accessing design tokens or doing layout have flattened system prop: Grid, Stack, Typography (e.g. fontWeight).
These should basically be the same as Box then, both flattened system prop + sx or styles. If that is the case I agree, and we should make sure we document all these components under the same section, so it would make sense to people why some components have them flattened and other don't...
There was a problem hiding this comment.
Yep, and I am saying system in case if we are going to use it as a simply collection of system props, if we are going to treat is as a superset of CSS
From my perspective, we should go with the superset of CSS for the standalone property. We would gain an improved version of the CSS prop without the limitations of the Box. I can't think of any real downside.
| @@ -31,6 +31,7 @@ export default function ContinuousSlider() { | |||
| </Grid> | |||
| <Grid item xs> | |||
| <Slider | |||
| system={{ color: 'secondary.main' }} | |||
There was a problem hiding this comment.
Changes will be reverted before merging
| /** | ||
| * Common system props. | ||
| */ | ||
| system: PropTypes.object, |
github-actions
bot
added
the
PR: out-of-date
oliviertassinari
added
new feature
|
On a related note, we might want to reimplement the system, maybe from scratch. There are a couple of elements to taken into accounts:
|
|
Closing in favor of #23205 |
This PR adds
systemprop on theSliderStyledcomponentRelated to #15561.