feat(material-experimental/mdc-dialog): switch to new theming api #23620
Conversation
andrewseguin
added
P2
google-cla
bot
added
the
cla: yes
andrewseguin
force-pushed
the
dialog-theme
branch
from
6bc712d to
1e153e2
Compare
| // Theme map with values for variables that will be overriden in the theme. | ||
| // MDC's theming system requires non-null values for the slots to be inserted | ||
| // and included as default values. | ||
| $dialog-initial-theme: ( |
There was a problem hiding this comment.
Do we actually need this? For the other components MDC provided a $light-theme with some default values that I used outside for the theme-styles mixin.
There was a problem hiding this comment.
Also if we do end up keeping this variable, the name should be with an underscore so that it isn't exported by Sass.
There was a problem hiding this comment.
Unfortunately yes - their variable contains the keys but with null values, which means their theme mixin skips inserting the slots. They plan to eventually provide a variable with filled values, but not until at least a couple quarters from now.
Changed variable name to underscore-prefix - thanks for catching that
There was a problem hiding this comment.
Here's their variable definition - https://github.com/material-components/material-components-web/blob/master/packages/mdc-dialog/_dialog-theme.scss#L36
They say that null (or any other falsy) values don't insert slots for backwards compatibility reasons. I wish that wasn't the case
There was a problem hiding this comment.
Hmm, that is unfortunate and a little fragile since we won't have a way of knowing when they introduce a new variable.
andrewseguin
force-pushed
the
dialog-theme
branch
from
1e153e2 to
2a6a53b
Compare
| // Theme map with values for variables that will be overriden in the theme. | ||
| // MDC's theming system requires non-null values for the slots to be inserted | ||
| // and included as default values. | ||
| $_dialog-initial-theme: ( |
There was a problem hiding this comment.
How much of this can we extract into a common location shared between multiple components? E.g. container-color and supporting-text-color seem like they'd be the common surface and on-surface colors for a lot of components.
andrewseguin
removed
the
cla: yes
googlebot
added
the
cla: yes
andrewseguin
added
target: minor
… api (angular#23620)" This reverts commit 353a548.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Applied the values for the theming by isolating the theme color/typography styles included in the current mixin. Should match the existing styles, and I'll run an internal presubmit to double-check this is a no-op