Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(snackbar): Update to match latest design guidelines #4166

Merged
merged 36 commits into from
Dec 13, 2018
Merged

Conversation

acdvorak
Copy link
Contributor

@acdvorak acdvorak commented Dec 11, 2018

Issues fixed

Screenshots

Baseline without action button

Baseline with action button

Stacked layout

BREAKING CHANGE: Snackbar's DOM and APIs have changed to match the latest design guidelines. See the Snackbar documentation for more information.

@acdvorak acdvorak added this to the v0.43.0 milestone Dec 11, 2018
@codecov-io
Copy link

codecov-io commented Dec 11, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@08e25f9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4166   +/-   ##
========================================
  Coverage          ?   98.5%           
========================================
  Files             ?     127           
  Lines             ?    5623           
  Branches          ?     745           
========================================
  Hits              ?    5539           
  Misses            ?      84           
  Partials          ?       0
Impacted Files Coverage Δ
packages/mdc-snackbar/foundation.js 100% <100%> (ø)
packages/mdc-snackbar/constants.js 100% <100%> (ø)
packages/mdc-snackbar/index.js 100% <100%> (ø)
packages/mdc-snackbar/util.js 100% <100%> (ø)

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 08e25f9...ee6a96d. Read the comment docs.

@mdc-web-bot

This comment has been minimized.

@mdc-web-bot

This comment has been minimized.

packages/mdc-snackbar/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-snackbar/_variables.scss Outdated Show resolved Hide resolved
$mdc-snackbar-fill-color: mix(mdc-theme-prop-value(on-surface), mdc-theme-prop-value(surface), 80%) !default;
$mdc-snackbar-label-ink-color: rgba(mdc-theme-prop-value(surface), mdc-theme-text-emphasis(high)) !default;
$mdc-snackbar-action-icon-ink-color: rgba(mdc-theme-prop-value(surface), mdc-theme-text-emphasis(high)) !default;
$mdc-snackbar-action-button-ink-color: #bb86fc !default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this color correspond to anything theme-related? Like a lighter version of primary? If so, encoding it as such should make it more flexible in relation to other Sass variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally yes, but our theme system doesn't support that, so Design told me to hard-code it 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might at least be more instructive to users looking to tweak variables (and less brittle at least as far as light-on-dark snackbars go) if this were expressed in terms of lighten($mdc-theme-primary)... except I can't actually get this color code as a result of it. It's close to 31% but not quite. Did design really give this exact value and if so where did they even get it from??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#bb86fc comes from the kickoff deck. I don't remember how Design came up with it.

I'm wary of using lighten() and darken() because they won't work for all colors. We would need a function like mdc-theme-light-variant() to ensure proper contrast ratios for accessibility purposes:

// Generate light and dark variants of the given color, offset by approximately
// the specified number of indexes within the color's tonal palette.
@function mdc-theme-tonal-variants_($color, $num-indexes: 2) {
$luminance: mdc-theme-luminance($color) * 100%;
$amount-1x: mdc-theme-clamp-percentage_($_mdc-theme-tonal-offset * $num-indexes);
$amount-2x: mdc-theme-clamp-percentage_($_mdc-theme-tonal-offset * $num-indexes * 2);
$lower-bound: $amount-1x;
$upper-bound: 100% - $lower-bound;
@if $luminance <= $lower-bound {
@return (
dark: lighten($color, $amount-1x),
light: lighten($color, $amount-2x)
);
} @else if $luminance >= $upper-bound {
@return (
dark: darken($color, $amount-2x),
light: darken($color, $amount-1x)
);
} @else {
@return (
dark: darken($color, $amount-1x),
light: lighten($color, $amount-1x)
);
}
}

However, the mdc-theme-light-variant() function was removed in #2473 because it didn't match the guidelines or the other platforms. I don't want to waste our time on something that will just get ripped out.

From my experience, hard-coding the color value is the least controversial option.
Implementing any kind of color logic is just going to open a huge can of worms all over again, which I have no interest in doing—especially when we're trying to get this thing merged before everyone goes on vacation.

If it's really that important to you, let's discuss it offline and create a separate follow up PR so this one doesn't get stuck in limbo forever.

packages/mdc-snackbar/constants.js Outdated Show resolved Hide resolved
Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget a BREAKING CHANGE: note in the commit description (something to the effect that the DOM and APIs have changed and to see the Snackbar documentation for more information)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment