-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4166 +/- ##
========================================
Coverage ? 98.5%
========================================
Files ? 127
Lines ? 5623
Branches ? 745
========================================
Hits ? 5539
Misses ? 84
Partials ? 0
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This reverts commit fbbc9f0.
This comment has been minimized.
This comment has been minimized.
🤖 Beep boop! Screenshot test report 🚦44 screenshots changed from Details44 Added: |
$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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷♂️
There was a problem hiding this comment.
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??
There was a problem hiding this comment.
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:
material-components-web/packages/mdc-theme/_functions.scss
Lines 72 to 97 in 13b5605
// 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.
… keep values in sync between Sass and JS
There was a problem hiding this 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)
🤖 Beep boop! Screenshot test report 🚦44 screenshots changed from Details44 Added: |
Issues fixed
mdc-snackbar-viewport-margin()
mixin)MDCSnackbar:closing
andMDCSnackbar:closed
events)close()
method)Screenshots
BREAKING CHANGE: Snackbar's DOM and APIs have changed to match the latest design guidelines. See the Snackbar documentation for more information.