fix(material/core): special-case icon button color token #31625
Conversation
andrewseguin
requested review from
wagnermaciel and
ok7sai
and removed request for
a team
andrewseguin
added
the
target: minor
| @@ -23,6 +23,7 @@ $candy-app-theme: mat.m2-define-light-theme(( | |||
|
|
|||
| @include mat.app-background(); | |||
| @include mat.elevation-classes(); | |||
| @include mat.m2-theme($candy-app-theme); | |||
There was a problem hiding this comment.
Why do we have this if there's also another theme?
There was a problem hiding this comment.
This only defines the system vars - I'd like to make a dev-app page that shows all the variables and this makes that work for both M2 and M3. This mixin, unlike M3, isn't meant to be a replacement for the individual component mixins
There was a problem hiding this comment.
Plus this helps validate that nothing changes on the site if it's included. This is how I found the issue with the icon button's styles.
Part of this is to encourage people to use this mixin and use systems vars instead of our m2 APIs to pull from the theme config. If we find an issue in the dev-app from this, we should fix it
| @@ -350,6 +350,21 @@ | |||
|
|
|||
| @include _define-m2-system-vars(m2.md-sys-shape-values(), $overrides); | |||
| @include _define-m2-system-vars(m2.md-sys-state-values(), $overrides); | |||
|
|
|||
There was a problem hiding this comment.
Since this is in system.scss, does it mean that we'll generate the concrete styles next to token styles?
There was a problem hiding this comment.
For the icon button? Yeah unfortunately. I thought about adding a modification in icon-button's theme mixin but that introduces the same sort of issue
There was a problem hiding this comment.
Should we at least have a @if (isM2) around it so it doesn't affect M3?
There was a problem hiding this comment.
Oh this is specifically an M2-only mixin, different entirely from the implementation for @mixin theme.
This mixin explicitly takes an M2 theme config and is meant to convert that to system CSS vars. I don't expect M3 apps to ever call this
Normally, an icon button should automatically get its color from whatever container it's placed in. This is important so the icon always has the right contrast to be visible, whether it's on a light, dark, or colored background.
However, when the icon button's M2 color token is
inherit, the browser will always use the fallback coloron-surface-variantwhich is going to now be defined in this mixin.To fix this, this code directly sets the icon's color and makes sure no fallback is defined. This forces the icon to correctly inherit its color from the container as originally intended.
Also: adds the mixin to the dev-app theme