[palette] Normalize the usage of the palette #9970
Conversation
oliviertassinari
force-pushed
the
docs-theme-details
branch
2 times, most recently
from
903c87a
to
bab2355
Compare
| @@ -24,7 +24,8 @@ const styles = theme => ({ | |||
| }, | |||
| }, | |||
| demo: theme.mixins.gutters({ | |||
| backgroundColor: theme.palette.background.contentFrame, | |||
| backgroundColor: | |||
| theme.palette.type === 'light' ? theme.palette.grey[200] : theme.palette.grey[900], | |||
There was a problem hiding this comment.
I'm not convinced theme.palette.grey should exist in the default theme. Two components (Button & ExpansionPanelSummary) are using it for a background color when that should be in theme.palette.background, and one, Switch, should probably be calculating its color some other way (I didn't look further).
In the docs we can just import grey directly.
There was a problem hiding this comment.
grey exists in the default theme because we already import it. It's mainly here for the ease of use. Let's see if we can remove it from the core components to start with :) .
There was a problem hiding this comment.
grey existe in the default theme because we already import it
Sure, but then we also import other colors, and they're not added to the theme.
Let's see if we can remove it from the core components
Sure, but in that case, lets not add it here.
There was a problem hiding this comment.
I won't remove the grey usage. I remember why we inject it now: so people can provide their own tones.
There was a problem hiding this comment.
@oliviertassinari. If we fix the couple of components using this to use more appropriate theme keys, users can still change them there.
Don't worry if you don't want to fix it here. I can handle it in a separate PR. One more line to fix in the docs won't make a difference.
There was a problem hiding this comment.
If we fix the couple of components using this to use more appropriate theme keys
Same, I have been going the other way around. the palette isn't a hub for all the configuration variables the components might need (we gonna need another solution for this). I think that we should aim for a palette as slim as possible. People shouldn't pay tooltip variable cost when only using a button. Also, it won't scale to the third party components. Code splitting is important. It's also simpler to document.
There was a problem hiding this comment.
the palette isn't a hub for all the configuration variables the components might need
Agreed.
(we gonna need another solution for this)
You've already documented a couple. Do we need anything more?
There was a problem hiding this comment.
You've already documented a couple. Do we need anything more?
@mbrookes I have been wondering about it since 2016 when @nathanmarks redesigned the styling & theming solution. I don't have the answer 🤔 .
oliviertassinari
force-pushed
the
docs-theme-details
branch
5 times, most recently
from
175150e
to
bbc7120
Compare
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
docs-theme-details
branch
from
bbc7120
to
b24b356
Compare
oliviertassinari
added
the
design: material
|
Piou, I'm done! |
color.charAt(0). Instead, we return the color and raise a warning.TableRowclasses withTableCellclasses.All of this started in my effort to document the theme variables. Turns out, the implementation was a mess. I have done my best to simplify it, going back and forth with the specification.
Closes #9922.
Breaking change
theme.palette.inputobject.theme.palette.text.iconcolor.theme.palette.background.contentFrame, it was only used in the documentation.theme.palette.text.dividertotheme.palette.divider, it's not a text color.theme.palette.text.lightDivider, there is no reference to is in the specification, better keep things simple.