feat(elevation): move elevation rules into theme stylesheets #11344

benelliott · 2018-05-15T17:32:56Z

Move all usages of elevation mixins out of component stylesheets and into their respective themes.
This allows the elevation color to be changed based on the theme. Add new mat-theme-elevation
and mat-theme-overridable-elevation shorthand mixins to apply the current theme's elevation
color. Replace all usages of mat-elevation mixins with themed equivalents. Add elevation
property to the default foreground theme palettes.

Closes #11343

BREAKING CHANGE:

Because mat-elevation usages have been moved out of component stylesheets, users who have
not invoked a theme mixin will not see any elevation shadows on Material components.
However, users that have created a custom theme which lacks the elevation property will
still see the default black shadows.

Additionally, users who want to use themed elevations in their custom components can create
their own shorthand mixin:

@import '~angular/material/theming';

$myTheme: ...

@mixin my-elevation($zValue) {
  @include mat-theme-elevation($zValue, $myTheme);
}

and then invoke angular-material-theme with the $myTheme variable.

benelliott · 2018-05-15T17:33:54Z

Example of using this feature in a custom theme - setting elevation to violet:

jelbourn · 2018-05-15T21:14:36Z

@benelliott are you saying this is only breaking when a theme is missing? If that's the case, it wouldn't be considered breaking because a theme has always been required.

benelliott · 2018-05-15T22:29:01Z

@jelbourn The situations I can think of are:

The user uses a built-in theme - these themes have the foreground.elevation property set to black so this is the same as before
The user uses a custom theme - the mat-theme-elevation mixin defaults the color to black if the theme map returns null so this is the same as before
The user uses no theme - in this case, there will be no shadow

So yes I think so, unless there is a situation I'm not thinking of!

jelbourn · 2018-05-15T22:52:53Z

src/lib/core/style/_elevation.scss

@@ -151,6 +151,14 @@ $_mat-elevation-prefix: 'mat-elevation-z';
              #{map-get(_get-ambient-map($color, $opacity), $zValue)};
 }

+@mixin mat-theme-elevation($zValue, $theme, $opacity: $mat-elevation-opacity) {


The new mixins need descriptions similar to the existing ones

jelbourn · 2018-05-15T22:55:18Z

src/lib/core/style/_elevation.scss

@@ -151,6 +151,14 @@ $_mat-elevation-prefix: 'mat-elevation-z';
              #{map-get(_get-ambient-map($color, $opacity), $zValue)};
 }

+@mixin mat-theme-elevation($zValue, $theme, $opacity: $mat-elevation-opacity) {
+  $foreground: map-get($theme, foreground);
+  $elevation-color: map-get($foreground, elevation);


Typically the background and foreground palettes are fixed regardless of theme; how would you go about changing the color of the shadow?

I took the position that wanting to change the elevation color was a fairly advanced use case, so it would be admissible for it to be changed by providing angular-material-theme with a fully custom theme map (as opposed to using the mat-light-theme and mat-dark-theme functions). This is what I do in a project where I want full control over the colors used in the app, however it has just occurred to me that it may not be part of the public API(?).

An alternative solution could be to provide it as an optional fourth argument to the mat-light-theme and mat-dark-theme functions (with default value black), and then to convert the $mat-light-theme-foreground and $mat-dark-theme-foreground variables into functions that take the elevation color as their only argument.

@josephperrott and I took a closer look at this, and found that we're going to have some cases in Google where it will be more common to want to customize the shadow color based on the theme colors, so we probably want to make it more ergonomic to customize the shadow based on that.

We'd also want to avoid adding a lot of excess css to the theme so most people wouldn't end up using. I'm still thinking about a good way to approach it.

@jelbourn Perhaps the additional parameter to the theme functions could be used instead?

An alternative way to avoid adding CSS to the themes could be to remove all internal usages of the mixins, and only to rely on the .mat-elevation-z? classes for applying elevation to Material components. This approach would actually reduce the amount of CSS overall, as only the output of

@for $zValue from 0 through 24 { .#{$_mat-elevation-prefix}#{$zValue} { @include mat-theme-elevation($zValue, $theme); } }

would be rendered (component stylesheets would be smaller as they no longer include the box shadow rules).

Perhaps overridable elevation could then be implemented like this:

$_mat-overridable-elevation-prefix: 'mat-overridable-elevation-z'; @for $zValue from 0 through 24 { .#{$_mat-overridable-elevation-prefix}#{$zValue}:not([class*='#{$_mat-elevation-prefix}']) { @include mat-theme-elevation($zValue, $theme); } }

and the old mat-overridable-elevation mixin would be deleted.

I like the idea of using the common set of css classes for everything. We would have to make sure they're still easily overridable, though.

I guess what this PR is really about is being able to customize the base shadow color, which makes sense to be in the foreground palette. If we want to set a shadow based on another palette, I don't see a way around explicitly passing the color in the component's theme file.

I think it boils down to two approaches:

1) Extend the current elevation mixins to support themes, and move their usages to the theme stylesheets (as in the current PR).

Advantages:

Code changes are minimal

CSS size increase is negligible

Disadvantages:

Moving elevation rules to the theme stylesheets means that everyone's CSS gets larger, regardless of whether they use those components

Size of elevation CSS will increase linearly with the number of components added to the library

2) Incorporate the theme color into the mat-elevation-z* CSS classes and only use these classes to apply elevation to components

Advantages:

CSS size will not increase as components are introduced to the library

CSS size may actually be reduced, as there is currently duplication from using the mixin in both the mat-elevation-z classes and the internal component stylesheets

Since elevation rules can only come from one set of classes, inspecting element elevation becomes simpler

Disadvantages:

Components will now have to directly apply their own elevation rules in TypeScript. This is potentially a major refactor - for example, MatButton will have to apply mat-elevation-z8 if it has the mat-raised-button attribute set. This logic in itself may be trivial but it would require a wave of new unit tests

Will probably have to remove the $_mat-elevation-prefix variable and just use the static string if elevation is applied statically as classes in component templates (alternately, could mirror this variable in TypeScript and only ever apply these classes in TypeScript within the library)

I'm happy to take a shot at the second option if you think it is more appropriate.

jelbourn · 2018-06-06T17:54:59Z

@crisbeto do you have any opinions on this? I would lean towards just making the elevation part of the theme solely because it's a simpler change

crisbeto · 2018-06-06T19:09:27Z

Keeping it in the theme sounds good. My only nit is that, IMO, the new mixins should be made private (prefixed with an underscore) since I don't see them being particularly useful for consumers.

ngbot · 2018-06-07T19:24:20Z

Hi @benelliott! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-06-07T19:28:04Z

Hi @benelliott! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

benelliott · 2018-06-09T12:05:34Z

Rebased and added suggestion from @crisbeto.

benelliott · 2018-06-22T19:11:28Z

@crisbeto Is there anything else you would like me to change before this can be merged?

crisbeto

LGTM. Leaving the final say to @jelbourn.

jelbourn · 2018-06-25T23:51:47Z

I'm running a presubmit of this PR against Google apps to make sure it doesn't cause any breakages

benelliott · 2018-06-26T07:57:34Z

Cool, thanks.

ngbot · 2018-07-12T14:47:34Z

Hi @benelliott! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mmalerba · 2018-07-16T19:53:29Z

@jelbourn what happened with the presubmit on this one, should we mark lgtm, merge-ready?

jelbourn · 2018-07-16T20:24:23Z

Presubmit was black across the board, need to update some build files for another pass
(I went through one round of this already but ended u getting merge conflicts and had to restart)

benelliott · 2018-07-17T08:29:27Z

Hi @jelbourn, I've just rebased to resolve the conflicts. Is black good or bad for a presubmit?

ngbot · 2018-07-17T16:48:08Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
missing required label: "target: *"
status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

mmalerba · 2018-09-10T16:02:02Z

@benelliott I've changed all of the internal components to only use elevation in their theme files, if you rebase I'll take another stab at getting this in

benelliott · 2018-09-11T16:32:33Z

@mmalerba Nice - rebased. Not sure why ngbot didn't ping me about the conflicts this time round.

ngbot · 2018-09-11T17:44:49Z

Hi @benelliott! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

mmalerba · 2018-09-13T18:39:20Z

src/lib/button/_button-base.scss

-  &:not([disabled]):active {
-    @include mat-overridable-elevation(8);
-  }
-
  &[disabled] {
    box-shadow: none;


I think this needs to move to the theme too, I'm seeing screenshots with disabled buttons that have shadows (this is basically the equivalent of mat-elevation(0))

card has a similar one that should go into the theme file:

&.mat-card-flat { box-shadow: none; }

@mixin

Move all usages of elevation mixins out of component stylesheets and into their respective themes. This allows the elevation color to be changed based on the theme. Add new `mat-theme-elevation` and `mat-theme-overridable-elevation` shorthand mixins to apply the current theme's elevation color. Replace all usages of `mat-elevation` mixins with themed equivalents. Add `elevation` property to the default `foreground` theme palettes. Closes angular#11343 BREAKING CHANGE: Because `mat-elevation` usages have been moved out of component stylesheets, users who have not invoked a theme mixin will not see any elevation shadows on Material components. However, users that have created a custom theme which lacks the `elevation` property will still see the default black shadows. Additionally, users who want to use themed elevations in their custom components can create their own shorthand mixin: ``` @import '~angular/material/theming'; $myTheme: ... @mixin my-elevation($zValue) { @include mat-theme-elevation($zValue, $myTheme); } ``` and then invoke `angular-material-theme` with the `$myTheme` variable.

benelliott · 2018-09-14T10:20:09Z

Hi @mmalerba, I've addressed your comments + rebased + given the flat card and disabled buttons a quick once-over on the demo app.

I decided to use elevation 0 instead of box-shadow: none to preserve the level of abstraction. Let me know if you want me to change that.

benelliott · 2018-09-18T21:18:53Z

🎉🎉🎉

* Currently if chips are disabled with the spec 2018 alignment, it's not clear whether a chip is disabled or not. The chip still has a focus effect, hover effect and pointer cursor for the `mat-chip-remove`. * Also moves the `@include mat-elevation` to the theme (probably introduced before: angular#11344 has been merged)

* Fixes a remaining elevation style which is not created inside of the theme. (related to: angular#11344)

* Currently if chips are disabled with the spec 2018 alignment, it's not clear whether a chip is disabled or not. The chip still has a focus effect, hover effect and pointer cursor for the `mat-chip-remove`. * Also moves the `@include mat-elevation` to the theme (probably introduced before: angular#11344 has been merged)

Fixes a remaining elevation style which is not created inside of the theme. (related to: #11344)

Fixes a remaining elevation style which is not created inside of the theme. (related to: angular#11344)

* Currently if chips are disabled with the spec 2018 alignment, it's not clear whether a chip is disabled or not. The chip still has a focus effect, hover effect and pointer cursor for the `mat-chip-remove`. * Also moves the `@include mat-elevation` to the theme (probably introduced before: #11344 has been merged)

* Currently if chips are disabled with the spec 2018 alignment, it's not clear whether a chip is disabled or not. The chip still has a focus effect, hover effect and pointer cursor for the `mat-chip-remove`. * Also moves the `@include mat-elevation` to the theme (probably introduced before: angular#11344 has been merged)

* Currently if chips are disabled with the spec 2018 alignment, it's not clear whether a chip is disabled or not. The chip still has a focus effect, hover effect and pointer cursor for the `mat-chip-remove`. * Also moves the `@include mat-elevation` to the theme (probably introduced before: #11344 has been merged)

angular-automatic-lock-bot · 2019-09-09T16:01:22Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

benelliott requested review from crisbeto, devversion, jelbourn, josephperrott, mmalerba and tinayuangao as code owners May 15, 2018 17:32

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 15, 2018

jelbourn reviewed May 15, 2018

View reviewed changes

benelliott force-pushed the elevation-theme branch from 265c4d3 to 3a5a245 Compare June 9, 2018 12:01

benelliott force-pushed the elevation-theme branch from 3a5a245 to 73bcbd1 Compare June 22, 2018 14:16

crisbeto approved these changes Jun 23, 2018

View reviewed changes

benelliott force-pushed the elevation-theme branch from 73bcbd1 to 89fe6d3 Compare July 17, 2018 08:28

mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 17, 2018

benelliott force-pushed the elevation-theme branch 2 times, most recently from 357e85b to a05cf07 Compare September 11, 2018 16:31

mmalerba added the P2 The issue is important to a large percentage of users, with a workaround label Sep 11, 2018

mmalerba force-pushed the elevation-theme branch 2 times, most recently from a42b68a to 6d1818d Compare September 12, 2018 15:49

mmalerba reviewed Sep 13, 2018

View reviewed changes

benelliott force-pushed the elevation-theme branch from 6d1818d to 995957c Compare September 14, 2018 10:18

mmalerba merged commit 9c075f5 into angular:master Sep 18, 2018

devversion mentioned this pull request Sep 23, 2018

fix(chips): disabled state not matching specs #13272

Merged

devversion added a commit to devversion/material2 that referenced this pull request Sep 23, 2018

fix(snackbar): move elevation style to snackbar theme

cbfe917

* Fixes a remaining elevation style which is not created inside of the theme. (related to: angular#11344)

devversion mentioned this pull request Sep 23, 2018

fix(snackbar): move elevation style to snackbar theme #13273

Merged

jelbourn added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Sep 24, 2018

jelbourn pushed a commit that referenced this pull request Sep 25, 2018

fix(snackbar): move elevation style to snackbar theme (#13273)

96e670f

Fixes a remaining elevation style which is not created inside of the theme. (related to: #11344)

roboshoes pushed a commit to roboshoes/material2 that referenced this pull request Oct 23, 2018

fix(snackbar): move elevation style to snackbar theme (angular#13273)

527afc1

Fixes a remaining elevation style which is not created inside of the theme. (related to: angular#11344)

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elevation): move elevation rules into theme stylesheets #11344

feat(elevation): move elevation rules into theme stylesheets #11344

benelliott commented May 15, 2018

benelliott commented May 15, 2018

jelbourn commented May 15, 2018

benelliott commented May 15, 2018

jelbourn May 15, 2018

jelbourn May 15, 2018

benelliott May 15, 2018 •

edited

Loading

jelbourn May 16, 2018

benelliott May 16, 2018 •

edited by jelbourn

Loading

jelbourn May 16, 2018

benelliott May 17, 2018

jelbourn commented Jun 6, 2018

crisbeto commented Jun 6, 2018

ngbot bot commented Jun 7, 2018

ngbot bot commented Jun 7, 2018

benelliott commented Jun 9, 2018

benelliott commented Jun 22, 2018

crisbeto left a comment

jelbourn commented Jun 25, 2018

benelliott commented Jun 26, 2018

ngbot bot commented Jul 12, 2018

mmalerba commented Jul 16, 2018

jelbourn commented Jul 16, 2018

benelliott commented Jul 17, 2018

ngbot bot commented Jul 17, 2018

mmalerba commented Sep 10, 2018

benelliott commented Sep 11, 2018

ngbot bot commented Sep 11, 2018

mmalerba Sep 13, 2018

mmalerba Sep 13, 2018

benelliott commented Sep 14, 2018

benelliott commented Sep 18, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019

feat(elevation): move elevation rules into theme stylesheets #11344

feat(elevation): move elevation rules into theme stylesheets #11344

Conversation

benelliott commented May 15, 2018

benelliott commented May 15, 2018

jelbourn commented May 15, 2018

benelliott commented May 15, 2018

jelbourn May 15, 2018

Choose a reason for hiding this comment

jelbourn May 15, 2018

Choose a reason for hiding this comment

benelliott May 15, 2018 • edited Loading

Choose a reason for hiding this comment

jelbourn May 16, 2018

Choose a reason for hiding this comment

benelliott May 16, 2018 • edited by jelbourn Loading

Choose a reason for hiding this comment

jelbourn May 16, 2018

Choose a reason for hiding this comment

benelliott May 17, 2018

Choose a reason for hiding this comment

1) Extend the current elevation mixins to support themes, and move their usages to the theme stylesheets (as in the current PR).

2) Incorporate the theme color into the mat-elevation-z* CSS classes and only use these classes to apply elevation to components

jelbourn commented Jun 6, 2018

crisbeto commented Jun 6, 2018

ngbot bot commented Jun 7, 2018

ngbot bot commented Jun 7, 2018

benelliott commented Jun 9, 2018

benelliott commented Jun 22, 2018

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn commented Jun 25, 2018

benelliott commented Jun 26, 2018

ngbot bot commented Jul 12, 2018

mmalerba commented Jul 16, 2018

jelbourn commented Jul 16, 2018

benelliott commented Jul 17, 2018

ngbot bot commented Jul 17, 2018

mmalerba commented Sep 10, 2018

benelliott commented Sep 11, 2018

ngbot bot commented Sep 11, 2018

mmalerba Sep 13, 2018

Choose a reason for hiding this comment

mmalerba Sep 13, 2018

Choose a reason for hiding this comment

benelliott commented Sep 14, 2018

benelliott commented Sep 18, 2018

angular-automatic-lock-bot bot commented Sep 9, 2019

benelliott May 15, 2018 •

edited

Loading

benelliott May 16, 2018 •

edited by jelbourn

Loading

2) Incorporate the theme color into the `mat-elevation-z*` CSS classes and only use these classes to apply elevation to components