feat(theme): Add support for CSS vars with fallback #4470

patrickrodee · 2019-03-01T20:40:33Z

What this does
Add the ability to specify a CSS custom property name and fallback for color mixins. Allows clients that can support CSS custom property theming to use that when desired without breaking browsers that do not support CSS custom properties.

What this does not do
This does not establish naming conventions for CSS custom properties. Those names are entirely up to the developers who use this feature.

Usage

.my-custom-button {
  @include mdc-button-fill((
    varname: --my-custom-button-fill,
    fallback: purple,
  ));
}

The generated CSS is as follows:

.my-custom-button {
  background-color: purple;
  background-color: var(--my-custom-button-fill, purple);
}

This CSS allows for browsers that do not support CSS custom properties to default to the fallback (purple) while still supporting an overridable CSS custom property --my-custom-button-fill.

mdc-web-bot · 2019-03-01T20:57:07Z

All 627 screenshot tests passed for commit 5d49c5a vs. master! 💯🎉

mdc-web-bot · 2019-03-01T22:46:18Z

All 627 screenshot tests passed for commit 7d985e2 vs. master! 💯🎉

mdc-web-bot · 2019-03-01T23:02:49Z

All 627 screenshot tests passed for commit fd51863 vs. master! 💯🎉

mdc-web-bot · 2019-03-04T18:07:01Z

All 627 screenshot tests passed for commit 195bcba vs. master! 💯🎉

…omponents/material-components-web into feat/theme/vars-with-fallback

mdc-web-bot · 2019-03-04T22:57:55Z

All 627 screenshot tests passed for commit a43e2ff vs. master! 💯🎉

mdc-web-bot · 2019-03-11T22:00:05Z

All 627 screenshot tests passed for commit bdebc93 vs. master! 💯🎉

abhiomkar · 2019-03-12T17:32:16Z

I think this is a great addition to add support for CSS custom property in overriding component styles! 👍

I've a suggestion in the way we use the mixin, instead of changing the style argument structure I added a way to auto detect the presence of CSS custom property and extract the fallback color automatically.

// theme/_mixins.scss

@mixin mdc-theme-css-var($property, $var, $important: false, $edgeOptOut: false) {
  @if is-css-var($var) {
    $fallback-style: get-fallback-style($var); // Extracts fallback color from css custom property.
    @include mdc-theme-prop($property, $fallback-style, $important: $important, $edgeOptOut: $edgeOptOut);
    @include mdc-theme-prop($property, $var, $important: $important, $edgeOptOut: $edgeOptOut);
  } @else {
    @include mdc-theme-prop($property, $var, $important: $important, $edgeOptOut: $edgeOptOut)
  }
}

// button/_mixins.scss

@mixin mdc-button-container-fill-color($color) {
  &:not(:disabled) {
    @include mdc-theme-css-var(background-color, $color, $edgeOptOut: true);
  }
}

// override.scss

.my-custom-button {
  @include mdc-button-container-fill-color(var(--my-custom-button-color, purple));
}

WDYT?

patrickrodee · 2019-03-12T17:43:07Z

@abhiomkar I originally thought about that but felt the results were a little too "magic." My intention was to make it very clear that a fallback value had to be specified and what role it served. The suggested change also has side-effects for anyone already passing var(--custom-prop, purple) to a mixin. I was hoping to avoid any side-effects with this change and make it purely opt-in.

mdc-web-bot · 2019-03-13T17:00:55Z

All 627 screenshot tests passed for commit f5dfc59 vs. master! 💯🎉

abhiomkar

Thanks for the clarification!

Aren't we already including fallback CSS with theme literals (primary, secondary etc)? Do you think adding an additional flag to mdc-theme-prop mixin (for example, fallback: false) would resolve opt-in issue?

packages/mdc-theme/_functions.scss

packages/mdc-theme/_mixins.scss

abhiomkar · 2019-03-13T19:34:53Z

packages/mdc-theme/_variables.scss

@@ -129,6 +129,10 @@ $mdc-theme-property-values: (
 //
 // NOTE: This function must be defined in _variables.scss instead of _functions.scss to avoid circular imports.
 @function mdc-theme-prop-value($style) {
+  @if mdc-theme-is-var-with-fallback_($style) {


Returning the new style type as is causes an issue to other mixins that uses mdc-theme-prop-value() function. For example, mdc-card-outline uses this function to set border-color. mdc-card/_mixins.scss:L303

Can we update that line to be as follows?

@include mdc-theme-prop(border-color, $color);

List divider uses the approach described above. I'll include that update in this PR.

material-components-web/packages/mdc-list/_mixins.scss

Line 360 in b9c5fc6

@include mdc-theme-prop(border-bottom-color, $color);

I'm confused though, what issue was this causing? Do we need to be concerned for existing valid cases of mdc-theme-prop-value being used?

It is still an issue if component needs to mix colors or apply opacity using rgba() sass function.

For example, dialog uses rgba() to apply opacity to color.

dialog/_mixins.scss:

background-color: rgba(mdc-theme-prop-value($color), $opacity);

Where, mdc-theme-prop-value returns new color format which rgba function doesn't support.

This is another advanced use case where text field uses mix() function two mix two colors:

textfield/_variables.scss:

$mdc-text-field-background: mix(mdc-theme-prop-value(on-surface), mdc-theme-prop-value(surface), 4%);

@kfranqueiro It shouldn't break existing valid usage. This is an issue only for users who send new color format to component's mixins.

mdc-dialog-title-ink-color('purple'); // works mdc-dialog-title-ink-color({ varname: '--custom-css-var-ink-color', fallback: purple }); // breaks since this applies opacity on given color using rgba

mdc-theme-prop-value has been updated to always return the fallback value.

material-components-web/packages/mdc-theme/_variables.scss

Lines 131 to 138 in e58979e

@function mdc-theme-prop-value($style) {

@if mdc-theme-is-var-with-fallback_($style) {

@return mdc-theme-get-var-fallback_($style);

}

@if mdc-theme-is-valid-theme-prop-value_($style) {

@return $style;

}

mdc-web-bot · 2019-03-13T21:01:44Z

All 627 screenshot tests passed for commit ef5d84f vs. master! 💯🎉

mdc-web-bot · 2019-03-13T21:11:43Z

All 627 screenshot tests passed for commit a0293a0 vs. master! 💯🎉

patrickrodee · 2019-03-13T21:18:10Z

@abhiomkar I think that would only work for the existing CSS custom properties (primary, secondary). I'm intending to support arbitrary CSS custom properties.

mdc-web-bot · 2019-03-13T21:32:16Z

All 627 screenshot tests passed for commit 65a61b0 vs. master! 💯🎉

mdc-web-bot · 2019-03-13T21:33:02Z

All 627 screenshot tests passed for commit e58979e vs. master! 💯🎉

abhiomkar

@patrickrodee That should work for arbitrary CSS custom properties too if mdc-theme-prop can detect the presence of CSS customer property and apply fallback CSS value with opt-in flag.

@include mdc-theme-prop(color, var(--my-custom-color, purple), fallback: true);

generates:

color: purple;
color: var(--my-custom-color, purple);

packages/mdc-card/_mixins.scss

abhiomkar · 2019-03-13T22:27:59Z

packages/mdc-theme/README.md

@@ -110,6 +110,38 @@ Property Name | Description
 `on-secondary` | A text/iconography color that is usable on top of secondary color
 `on-surface` | A text/iconography color that is usable on top of surface color

+#### `mdc-theme-prop` with CSS Custom Properties
+
+> **Note** The Sass map `$style` argument is intended *only* for use with color mixins.


I don't see why we can't use it for other CSS properties. We aren't doing anything specific with color in our implementation?

I haven't tested it for other CSS custom properties. I'd rather scope this to what it was intended for. We can always remove this line from the docs at a later date.

packages/mdc-theme/README.md

abhiomkar · 2019-03-13T22:39:47Z

packages/mdc-theme/README.md

+
+```
+.foo {
+  @include mdc-theme-prop(color, (


If I understand this correctly, this is equivalent to applying same mixin with CSS custom property and regular CSS value?

mdc-button-fill-color(purple); mdc-button-fill-color(var(--my-custom-button, purple));

The output CSS is pretty different. Calling both mixins includes the full selector twice. Below is an approximation of the output CSS:

.mdc-button { background-color: purple; } .mdc-button { background-color: var(--my-custom-button, purple); }

mdc-web-bot · 2019-03-14T00:23:17Z

All 627 screenshot tests passed for commit 5643f95 vs. master! 💯🎉

mdc-web-bot · 2019-03-14T16:37:31Z

All 627 screenshot tests passed for commit b92a208 vs. master! 💯🎉

abhiomkar

LGTM.

…al-components#4470) (cherry picked from commit 0bfb393)

feat(theme): Add support for CSS vars with fallback

5d49c5a

patrickrodee added the discussion label Mar 1, 2019

googlebot added the cla: yes label Mar 1, 2019

WIP fix failing CSS lint

7d985e2

Merge branch 'master' into feat/theme/vars-with-fallback

fd51863

Merge branch 'master' into feat/theme/vars-with-fallback

195bcba

patrickrodee added 2 commits March 4, 2019 14:41

WIP use "varname" instead of "name"

7c1cd33

Merge branch 'feat/theme/vars-with-fallback' of github.com:material-c…

a43e2ff

…omponents/material-components-web into feat/theme/vars-with-fallback

patrickrodee removed the discussion label Mar 11, 2019

Merge branch 'master' into feat/theme/vars-with-fallback

bdebc93

Merge branch 'master' into feat/theme/vars-with-fallback

45161a9

Merge branch 'master' into feat/theme/vars-with-fallback

f5dfc59

abhiomkar suggested changes Mar 13, 2019

View reviewed changes

patrickrodee added 2 commits March 13, 2019 13:44

WIP add alternate tags

ef5d84f

WIP update from review

a0293a0

patrickrodee added 2 commits March 13, 2019 14:15

WIP updated readme

65a61b0

Merge branch 'master' into feat/theme/vars-with-fallback

e58979e

abhiomkar suggested changes Mar 13, 2019

View reviewed changes

WIP revert list change

5643f95

WIP update readme

b92a208

patrickrodee requested a review from abhiomkar March 14, 2019 16:21

abhiomkar approved these changes Mar 14, 2019

View reviewed changes

patrickrodee merged commit 0bfb393 into master Mar 14, 2019

patrickrodee deleted the feat/theme/vars-with-fallback branch March 14, 2019 21:47

adrianschmidt pushed a commit to Lundalogik/material-components-web that referenced this pull request Mar 29, 2019

feat(theme): Add support for arbitrary CSS vars with fallback (materi…

b4b954b

…al-components#4470) (cherry picked from commit 0bfb393)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme): Add support for CSS vars with fallback #4470

feat(theme): Add support for CSS vars with fallback #4470

patrickrodee commented Mar 1, 2019 •

edited

Loading

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 4, 2019

mdc-web-bot commented Mar 4, 2019

mdc-web-bot commented Mar 11, 2019

abhiomkar commented Mar 12, 2019

patrickrodee commented Mar 12, 2019

mdc-web-bot commented Mar 13, 2019

abhiomkar left a comment

abhiomkar Mar 13, 2019

patrickrodee Mar 13, 2019

patrickrodee Mar 13, 2019

kfranqueiro Mar 13, 2019

abhiomkar Mar 13, 2019

abhiomkar Mar 13, 2019 •

edited

Loading

patrickrodee Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

patrickrodee commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

abhiomkar left a comment

abhiomkar Mar 13, 2019

patrickrodee Mar 14, 2019

abhiomkar Mar 13, 2019

patrickrodee Mar 14, 2019

mdc-web-bot commented Mar 14, 2019

mdc-web-bot commented Mar 14, 2019

abhiomkar left a comment

	@function mdc-theme-prop-value($style) {
	@if mdc-theme-is-var-with-fallback_($style) {
	@return mdc-theme-get-var-fallback_($style);
	}

	@if mdc-theme-is-valid-theme-prop-value_($style) {
	@return $style;
	}

feat(theme): Add support for CSS vars with fallback #4470

feat(theme): Add support for CSS vars with fallback #4470

Conversation

patrickrodee commented Mar 1, 2019 • edited Loading

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 1, 2019

mdc-web-bot commented Mar 4, 2019

mdc-web-bot commented Mar 4, 2019

mdc-web-bot commented Mar 11, 2019

abhiomkar commented Mar 12, 2019

patrickrodee commented Mar 12, 2019

mdc-web-bot commented Mar 13, 2019

abhiomkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhiomkar Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

patrickrodee commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

mdc-web-bot commented Mar 13, 2019

abhiomkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Mar 14, 2019

mdc-web-bot commented Mar 14, 2019

abhiomkar left a comment

Choose a reason for hiding this comment

patrickrodee commented Mar 1, 2019 •

edited

Loading

abhiomkar Mar 13, 2019 •

edited

Loading