Add better support for custom icon colors #4079
Conversation
| top: 0; | ||
| width: units($theme-alert-bar-width); | ||
| } | ||
| border-left: units($theme-alert-bar-width) solid color("base-light"); |
There was a problem hiding this comment.
Change alert bar to a border for simplicity
There was a problem hiding this comment.
We should move this up before position: relative.
| @@ -1,3 +1,39 @@ | |||
| $alert-slim-icon-size: units(3); | |||
There was a problem hiding this comment.
We'll be adding this icon with a :before block and the mask technique instead of a background image on the alert itself, so some vars and calculations need to change.
There was a problem hiding this comment.
We might need a $alert-slim-icon-padding. The icon on slim variation stays the same but the padding varies when the icon size is updated.
For example, change $theme-alert-icon-size to 6.
| } | ||
|
|
||
| .usa-alert__body { | ||
| display: table-cell; | ||
| vertical-align: top; | ||
| @include u-padding-x($theme-alert-padding-x); |
There was a problem hiding this comment.
Remove the weird table-related styling
| @@ -2,23 +2,20 @@ | |||
| $bgcolor: if($name != "emergency", "#{$name}-lighter", $name); | |||
| $banner-text-color-token: get-color-token-from-bg( | |||
| $bgcolor, | |||
| $context: "Alert text" | |||
| $theme-alert-text-reverse-color, | |||
There was a problem hiding this comment.
Use theme colors
| $ie11-variant: get-color-token-from-bg($contrast-bg, "white", "black"); | ||
| $filename-ie11: if( | ||
| $ie11-variant == "white", | ||
| "usa-icons-bg/#{$filename-base}--white.svg", | ||
| "usa-icons/#{$filename-base}.svg" | ||
| ); | ||
|
|
||
| $image-props: url("#{$path}/#{$filename}") no-repeat center / #{$width} #{$height}; | ||
| $mask-props: url("#{$path}/#{$filename-base}.svg") no-repeat center / #{$width} | ||
| #{$height}; | ||
| $image-props: url("#{$path}/#{$filename-ie11}") no-repeat center / #{$width} #{$height}; |
There was a problem hiding this comment.
Fix this code so it pulls the right --white variant from usa-icons-bg for IE support
| @@ -27,6 +27,7 @@ $theme-alert-bar-width: 1 !default; | |||
| $theme-alert-font-family: "ui" !default; | |||
| $theme-alert-icon-size: 5 !default; | |||
| $theme-alert-padding-x: 2.5 !default; | |||
| $theme-alert-padding-y: 2 !default; | |||
There was a problem hiding this comment.
Add explicit padding-y setting instead of calculating it from padding-x, which isn't expected.
| // Position centered in the alert | ||
| bottom: 0; | ||
| content: ""; | ||
| display: block; | ||
| // padding - optical spacing value | ||
| left: $alert-icon-optical-padding; | ||
| position: absolute; | ||
| height: auto; | ||
| top: 0; |
There was a problem hiding this comment.
This changes the icon positioning a bit, so the icon is vertically centered on the alert.
There was a problem hiding this comment.
IE11 doesn't support calc() on generated content.
Maybe we should try a left value before this one or something like:
// Some left value for ie11
left: 1rem;
// Calc for modern browsers
@supports (left: $alert-icon-optical-padding) {
left: $alert-icon-optical-padding;
}
There was a problem hiding this comment.
It doesn't look like the calc() is working for the content padding-left either
There was a problem hiding this comment.
Just removed the calc()s and used Sass math instead
thisisdano
changed the title
| $theme-alert-text-color: default !default; | ||
| $theme-alert-text-reverse-color: default !default; |
There was a problem hiding this comment.
Should we deprecate these?
We could use the defaults instead. I know they're set to default, but I wonder if these are even used.
$theme-text-color
$theme-text-reverse-color
There was a problem hiding this comment.
They're maybe a little edge-y but if you wanted a different color for alerts or reverse alerts it would be helpful. I'd prefer not to deprecate them one release after introducing them, I guess. 😬
Fixes #4064