feat(snackbar): Update styles to match guidelines #4070

acdvorak · 2018-11-09T03:02:55Z

This PR also deletes snackbar's JS files and unit tests to avoid spurious test failures.
They will be added back in the next PR (for issue #4062).

Fixes #4061

codecov-io · 2018-11-09T03:12:39Z

Codecov Report

Merging #4070 into feat/snackbar will increase coverage by 0.06%.
The diff coverage is n/a.

@@                Coverage Diff                @@
##           feat/snackbar    #4070      +/-   ##
=================================================
+ Coverage          98.67%   98.74%   +0.06%     
=================================================
  Files                126      123       -3     
  Lines               5596     5406     -190     
  Branches             746      724      -22     
=================================================
- Hits                5522     5338     -184     
+ Misses                74       68       -6

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be887cb...5bd2940. Read the comment docs.

mdc-web-bot · 2018-11-09T03:19:53Z

🤖 Beep boop!

Screenshot test report 🚦

52 screenshots changed from feat/snackbar on commit 67de0ef:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/11/09/03_16_33_990/report/report.html?utm_source=github&utm_medium=pr_comment

Details

52 Added:

abhiomkar · 2018-11-09T20:30:07Z

packages/mdc-snackbar/_mixins.scss

+}
+
+@mixin mdc-snackbar-min-width($min-width) {
+  .mdc-snackbar__surface {


This is regarding additional wrapper .mdc-snackbar__surface used to achieve opaque background.

Did you run into any issues using mix() sass function?

Great question!

~~Two~~ One issues:

mix() prevents clients from using CSS variables

If the overlay color has an alpha channel, mix() will return an rgba() value.
This will make the snackbar's background appear semi-transparent instead of opaque, which violates the spec.
Ok, this was a stupid argument. It probably doesn't make sense for the overlay color to have an alpha channel.

do we need CSS variable support for users using sass mixins?

If possible, yes, but if we have to choose between supporting CSS variables and having a simple Sass API, we normally go with a simple Sass API.

I'll remove the __overlay element and simplify the fill-color mixin so that it only takes 1 color parameter. The mix() function will be called in _variables.scss, similar to what mdc-chips does.

abhiomkar · 2018-11-09T20:30:33Z

packages/mdc-snackbar/_mixins.scss

+  }
+}
+
+@mixin mdc-snackbar-surface-fill-color($color) {


mdc-snackbar-fill-color?

Should we expose two separate mixins? Seems like these mixins are helping customize low-level elements of this component.

I definitely see your point. Having two separate mixins is not very intuitive for clients who just want to set the background-color to a specific hex value.

However, the internal guidelines say that the rendered background-color of a snackbar should be a combination of two theme colors: surface and overlay. It may be low-level, but it's how the theming system works. I'm not sure what the alternative is.

Would it be better to have a single mixin that accepts two color params? E.g.:

@mixin mdc-snackbar-fill-color( $surface-color: surface, $overlay-color: on-surface, $overlay-opacity: high ) { .mdc-snackbar__surface { @include mdc-theme-prop(background-color, $surface-color); } .mdc-snackbar__overlay { $overlay-opacity-value: if( type-of($overlay-opacity) == number, $overlay-opacity, mdc-theme-text-emphasis($overlay-opacity) ); $overlay-color-value: rgba( mdc-theme-prop-value($overlay-color), $overlay-opacity-value ); @include mdc-theme-prop(background-color, $overlay-color-value); } }

If we decide to stick with two separate mixins, I probably wouldn't want to rename them, because IMO it would be confusing to have:

mdc-snackbar-fill-color (targets __surface)

mdc-snackbar-overlay-color (targets __overlay)

The name mdc-snackbar-fill-color would be misleading, because it doesn't actually set the rendered fill color of the component—just the background behind the overlay.

WDYT? Is there an alternative I'm not seeing here?

I'd be agreeable to one mixin w/ arguments for both colors. Separate mixins seems prone to people accidentally overlooking one or the other and not realizing they influence each other.

abhiomkar · 2018-11-09T22:26:35Z

test/screenshot/golden.json

@@ -1077,6 +1077,123 @@
      "desktop_windows_ie@11": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/10/23/19_08_56_617/spec/mdc-select/mixins/shape-radius.html.windows_ie_11.png"
    }
  },
+  "spec/mdc-snackbar/classes/baseline-with-action.html": {


Snackbar is being clipped in iPhone 5 viewport.

Wow, good catch! 👍

That's due to the default min-width value of 344px from the spec:

https://material.io/design/components/snackbars.html#spec

I'll follow up with Design on this.

(OOC, why did you think to test it on an iPhone 5? Never would have occurred to me 😝)

I was testing if snackbar snaps to screen edges for smaller browsers 🙂

abhiomkar · 2018-11-09T22:31:55Z

packages/mdc-snackbar/mdc-snackbar.scss

+
+  .mdc-snackbar--stacked & {
+    flex-direction: column;
+    align-items: flex-start;


Should this flip for RTL?

Nope! That's the beauty of flexbox and flex-start: It's already RTL-aware! 🎉

BTW, you can add ?dir=rtl to any screenshot test page URL to see what it looks like in RTL:

https://storage.googleapis.com/mdc-web-screenshot-tests/advorak/2018/11/09/02_51_51_193/spec/mdc-snackbar/index.html?dir=rtl

LTR:

RTL:

kfranqueiro · 2018-11-12T02:47:22Z

packages/mdc-snackbar/_mixins.scss

+}
+
+@mixin mdc-snackbar-shape-radius($radius) {
+  // The mdc-snackbar__overlay selector is needed to prevent the overlay's


You say the snackbar__overlay selector is needed... but it's not there?

Good catch, removed

kfranqueiro · 2018-11-12T02:49:44Z

packages/mdc-snackbar/_mixins.scss

+  }
+}
+
+@mixin mdc-snackbar-surface-fill-color($color) {


I'd be agreeable to one mixin w/ arguments for both colors. Separate mixins seems prone to people accidentally overlooking one or the other and not realizing they influence each other.

kfranqueiro · 2018-11-12T02:51:14Z

packages/mdc-snackbar/_variables.scss

+// Private
+//
+
+$mdc-snackbar-enter-delay_: 0ms;


Is there a reason these are distinguished as private, given the request we've gotten to expose everything as overrideable?

Good call—made them "public"

kfranqueiro · 2018-11-12T02:52:09Z

packages/mdc-snackbar/mdc-snackbar.scss

@@ -1,5 +1,5 @@
 //
-// Copyright 2017 Google Inc.
+// Copyright 2018 Google Inc.


We typically don't change the copyright year in pre-existing files. I can buy the argument in other cases that the files are effectively being replaced, but this one especially seems somewhat intact.

I actually rewrote this file from scratch, so I figured that needed a year bump

kfranqueiro · 2018-11-12T02:55:51Z

packages/mdc-snackbar/package.json

@@ -16,7 +16,12 @@
  "dependencies": {
    "@material/animation": "^0.41.0",
    "@material/base": "^0.41.0",
+    "@material/button": "^0.41.0",
+    "@material/icon-button": "^0.41.0",
+    "@material/dom": "^0.41.0",


I take it you're expecting to use this in the JS portion?

Yep, I just forgot to remove it from this PR. Fixed!

…`!default`

mdc-web-bot · 2018-11-12T18:06:30Z

🤖 Beep boop!

Screenshot test report 🚦

48 screenshots changed from feat/snackbar on commit dcb90e4:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/11/12/18_03_51_696/report/report.html?utm_source=github&utm_medium=pr_comment

Details

48 Added:

abhiomkar

Looks good overall! Added few comments mostly nit.

Are you planning to update README file in your next PR?

abhiomkar · 2018-11-12T17:44:50Z

packages/mdc-snackbar/_mixins.scss

+}
+
+@mixin mdc-snackbar-min-width($min-width) {
+  .mdc-snackbar__surface {


do we need CSS variable support for users using sass mixins?

abhiomkar · 2018-11-12T18:05:29Z

packages/mdc-snackbar/_mixins.scss

+}
+
+@mixin mdc-snackbar-fill-color($overlay-color, $overlay-opacity, $surface-color: surface) {
+  .mdc-snackbar__surface {


surface is a global override and can be customized using sass variable - $mdc-theme-surface. User may not need to send surface color here again?

I'm assuming the overlay color should blend with the global surface color which might be rendered behind snackbar from other components.

Individual components can technically have their own independent "surface" color, which may or may not be the same as the global surface color.

I checked with designer today, Snackbar doesn't seem to allow customization for snackbar container (surface).

Internal customers need a different background color, so we still need to at least provide a mixin to change it. But I'll remove the __overlay element and simplify the fill-color mixin so that it only takes 1 parameter.

abhiomkar · 2018-11-12T18:20:49Z

test/screenshot/golden.json

@@ -1077,6 +1077,123 @@
      "desktop_windows_ie@11": "https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/10/23/19_08_56_617/spec/mdc-select/mixins/shape-radius.html.windows_ie_11.png"
    }
  },
+  "spec/mdc-snackbar/classes/baseline-with-action.html": {


I was testing if snackbar snaps to screen edges for smaller browsers 🙂

abhiomkar · 2018-11-12T18:23:39Z

packages/mdc-snackbar/mdc-snackbar.scss

-  justify-content: flex-start;
-  margin-top: -12px;
-  margin-bottom: 8px;
+  display: none;


Do we still need to set this to display: none when the root has aria-hidden="true"?

Yes, display: none is necessary to prevent the elements from being focusable when the user presses the Tab key.

kfranqueiro · 2018-11-12T18:32:59Z

test/screenshot/spec/mdc-snackbar/classes/wide.html

+<html lang="en">
+  <head>
+    <meta charset="utf-8">
+    <title>Baseline Snackbar with Action - MDC Web Screenshot Test</title>


Update title?

kfranqueiro · 2018-11-12T18:34:50Z

test/screenshot/spec/mdc-snackbar/fixture.scss

-@import "../packages/mdc-textfield/mdc-text-field";
+@import "../../../../packages/mdc-snackbar/mixins";
+@import "../../../../packages/mdc-theme/color-palette";
+@import "../mixins";


We don't seem to be using this here. Should we remove it?

kfranqueiro · 2018-11-12T18:38:27Z

packages/mdc-snackbar/mdc-snackbar.scss

 @import "@material/rtl/mixins";
+@import "@material/theme/functions";


Are all of these theme imports used? I don't see mdc-theme anywhere in this file. I think they're used in _mixins instead.

kfranqueiro · 2018-11-12T18:39:43Z

packages/mdc-snackbar/mdc-snackbar.scss

+  // > To disable tap highlighting, set the alpha value to 0 (invisible).
+  // > If you set the alpha value to 1.0 (opaque), the element is not visible when tapped.
+  // See https://github.com/ben-eb/postcss-colormin/issues/1
+  -webkit-tap-highlight-color: rgba(0, 0, 0, 0);


Why is this needed on the entire snackbar element?

For some reason, iOS Safari displays a tap highlight on the entire snackbar element 🤷‍♀️

kfranqueiro · 2018-11-12T18:42:41Z

packages/mdc-snackbar/mdc-snackbar.scss

  display: flex;
+
+  // Non-static positioning is required to make `overflow: hidden` work in Safari.


Are we really running into this bug? Wouldn't we want relative positioning anyway because the overlay element is absolutely-positioned inside the surface element?

Now that I've removed the __overlay element and simplified the fill-color mixin to only take 1 parameter, we don't need this anymore. Removed!

kfranqueiro · 2018-11-12T18:44:47Z

packages/mdc-snackbar/mdc-snackbar.scss

+    pointer-events: auto; // Allow mouse events on surface element while snackbar is open
+  }
+
+  .mdc-snackbar--closing & {


Can you explain why there's a --closing but no --opening? It's probably hard to surmise without having the JS APIs to actually cause animation.

Opening and closing use different animations, so I need a separate class for --closing.
The opening animation is defined in --open.

kfranqueiro · 2018-11-12T18:46:32Z

packages/mdc-snackbar/mdc-snackbar.scss

-  margin: 0;
+// Two selectors are needed to increase specificity above `.material-icons`.
+// stylelint-disable-next-line selector-class-pattern
+.mdc-snackbar__action-icon.material-icons {


Should this really be using material-icons in the selector? What would happen if someone used an image or svg? Should we do the same-class-twice trick instead?

Same-class-twice trick it is!

abhiomkar

LGTM. Please address Ken's comments.

mdc-web-bot · 2018-11-13T02:55:49Z

🤖 Beep boop!

Screenshot test report 🚦

48 screenshots changed from feat/snackbar on commit 5bd2940:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/11/13/02_52_21_789/report/report.html?utm_source=github&utm_medium=pr_comment

Details

48 Added:

feat(snackbar): Update styles to match guidelines

67de0ef

Fixes #4061

acdvorak added this to the R21 (v0.42.0) milestone Nov 9, 2018

acdvorak assigned abhiomkar and kfranqueiro Nov 9, 2018

acdvorak requested review from abhiomkar and kfranqueiro November 9, 2018 03:02

googlebot added the cla: yes label Nov 9, 2018

abhiomkar requested changes Nov 9, 2018

View reviewed changes

kfranqueiro suggested changes Nov 12, 2018

View reviewed changes

acdvorak added 3 commits November 12, 2018 09:34

WIP: combine fill-color mixins; remove default opacity values; add …

9a78014

…`!default`

WIP: Remove unused mdc-dom dep

36dc815

WIP: Update flaky Edge screenshot

dcb90e4

abhiomkar requested changes Nov 12, 2018

View reviewed changes

kfranqueiro reviewed Nov 12, 2018

View reviewed changes

WIP: Simplify fill-color and ink-color mixins

a871ff0

abhiomkar approved these changes Nov 13, 2018

View reviewed changes

acdvorak added 2 commits November 12, 2018 17:48

WIP: Address PR comments

cfd3040

WIP: Redact flaky button text in Edge in elevation.html

0a6f375

kfranqueiro approved these changes Nov 13, 2018

View reviewed changes

WIP: Redact buttons on all pages in Edge to avoid flaky diffs

5bd2940

acdvorak merged commit fa2bc17 into feat/snackbar Nov 13, 2018

acdvorak deleted the feat/snackbar--styles branch November 13, 2018 02:56

abhiomkar mentioned this pull request Nov 27, 2018

Left-aligned snackbar appears under drawer #4113

Closed

		@import "@material/rtl/mixins";
		@import "@material/theme/functions";

		display: flex;

		// Non-static positioning is required to make `overflow: hidden` work in Safari.

feat(snackbar): Update styles to match guidelines #4070

feat(snackbar): Update styles to match guidelines #4070

Conversation

acdvorak commented Nov 9, 2018

codecov-io commented Nov 9, 2018 • edited Loading

Codecov Report

mdc-web-bot commented Nov 9, 2018

Screenshot test report 🚦

52 Added:

Choose a reason for hiding this comment

acdvorak Nov 10, 2018 • edited Loading

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

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

LTR:

RTL:

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

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 Nov 12, 2018

Screenshot test report 🚦

48 Added:

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

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

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

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 left a comment

Choose a reason for hiding this comment

mdc-web-bot commented Nov 13, 2018

Screenshot test report 🚦

48 Added:

codecov-io commented Nov 9, 2018 •

edited

Loading

acdvorak Nov 10, 2018 •

edited

Loading