feat(checkbox): Add color theme mixins and update default color to secondary #1365
Conversation
This gives the color rules a slightly higher CSS specificy, unfortunately, but as long as clients use our color mixins, it shouldn't be a problem.
packages/mdc-checkbox/_mixins.scss
Outdated
| @mixin mdc-checkbox-color($color) { | ||
| $ripple-config: (base-color: $color); | ||
|
|
||
| // TODO(acdvorak): Refactor ripple mixin to do this @if check automatically, or use mdc-theme-prop() |
There was a problem hiding this comment.
Yeah, I want to do something about ripple's base-color/theme-style split...
| // The frame's ::before element is used as a focus indicator for the checkbox | ||
| &::before { | ||
| @include mdc-checkbox-cover-element; | ||
| @include mdc-theme-prop(background, primary); |
There was a problem hiding this comment.
FYI, #1364 is adding $edgeOptOut: true here, so you will want to do likewise wherever the equivalent is (not sure if that will require doing it in all invocations of a mixin now, which will generate more CSS...)
…-mixins # Conflicts: # packages/mdc-checkbox/mdc-checkbox.scss
# Conflicts: # packages/mdc-checkbox/mdc-checkbox.scss
Remove `[aria-disabled="true"]` selectors, as they don't appear to be used or necessary.
packages/mdc-checkbox/_mixins.scss
Outdated
| } | ||
| } | ||
|
|
||
| @mixin mdc-checkbox-ripple-color($color, $opacity: .14) { |
There was a problem hiding this comment.
just take ripple-config as an input. I don't want to invest in ripple customization right now...we'll handle that with states.
You can make a public variable mdc-checkbox-ripple-opacity, and reference it from the demo code (thats what we did for button).
There was a problem hiding this comment.
That would make the API inconsistent with the other color mixins (which only take a $color param).
If all the mixins take a single $color param (and possibly an optional second param), it makes it easy for users to try all of our mixins without needing to read the documentation.
There was a problem hiding this comment.
With button we just take in ripple-config. Ripple config is an existing concept in MDD Web, with its own complications. Our work on states should help improve the state of ripple-config, but lets not tackle that same problem in this PR.
Also when we tried to use a $color param directly with the mdc-button-ripple mixin we hit a lot of weird issues (around theme-ing versus non-theme colors) and I don't want to run into those again.
Also also, it should be just mdc-checkbox-ripple
demos/checkbox.scss
Outdated
|
|
||
| $demo-custom-color: $material-color-red-500; | ||
|
|
||
| .demo-checkbox--custom { |
There was a problem hiding this comment.
.mdc-checkbox.demo-checkbox--custom
There was a problem hiding this comment.
In our demos the demo CSS comes after the MDC Web CSS....but on some clients order guarantee is difficult to implement. We want to document how to handle that case, which is to have a more specific CSS selector (two classes).
packages/mdc-checkbox/_mixins.scss
Outdated
| } | ||
|
|
||
| @mixin mdc-checkbox-stroke-color($color) { | ||
| .mdc-checkbox__background { |
There was a problem hiding this comment.
Can you make this two mixins? @mixin mdc-checkbox-filled-stroke-color:
@include mdc-checkbox-background-filled-enabled {
.mdc-checkbox__background {
@include mdc-theme-prop(border-color, $color);
}
}
And @mixin mdc-checkbox-unfilled-stroke-color:
@include mdc-checkbox-background-unfilled-enabled {
.mdc-checkbox__background {
@include mdc-theme-prop(border-color, $color);
}
}
And we dont want to let clients customize the stroke color when the checkbox is disabled (thats what we did for button too).
packages/mdc-checkbox/_mixins.scss
Outdated
| } | ||
|
|
||
| @mixin mdc-checkbox-container-color($color) { | ||
| .mdc-checkbox__background { |
There was a problem hiding this comment.
Can you make @mixin mdc-checkbox-container-color:
@include mdc-checkbox-background-filled-enabled {
.mdc-checkbox__background {
@include mdc-theme-prop(background-color, $color);
}
}
I can see why we wouldn't want to let clients customize the container color when the checkbox is disabled...But can you customize container color when the checkbox background is "unfilled"? (I'm guessing there is no container when the background is unfilled?)
packages/mdc-checkbox/_mixins.scss
Outdated
| } | ||
| } | ||
|
|
||
| @mixin mdc-checkbox-focus-color($color) { |
There was a problem hiding this comment.
mdc-checkbox-focus-indicator-color
| mdc-checkbox-transition-enter(transform, 0ms, 80ms); | ||
| opacity: .26; | ||
| @include mdc-checkbox-background-filled-disabled { | ||
| @include mdc-checkbox-stroke-color(transparent); |
There was a problem hiding this comment.
just reference border-color directly.
| opacity: .26; | ||
| @include mdc-checkbox-background-filled-disabled { | ||
| @include mdc-checkbox-stroke-color(transparent); | ||
| @include mdc-checkbox-container-color($mdc-checkbox-disabled-color); |
There was a problem hiding this comment.
just reference background-color directly.
| stroke-dashoffset: 0; | ||
| } | ||
| @include mdc-checkbox-background-unfilled-disabled { | ||
| @include mdc-checkbox-stroke-color($mdc-checkbox-disabled-color-dark); |
There was a problem hiding this comment.
just reference border-color directly
| transform: scaleX(1) rotate(0deg); | ||
| opacity: 1; | ||
| @include mdc-checkbox-background-filled-disabled { | ||
| @include mdc-checkbox-container-color($mdc-checkbox-disabled-color-dark); |
There was a problem hiding this comment.
just reference background-color directly
| } | ||
| } | ||
|
|
||
| // Needed to disable hover effects on CSS-only (non-JS) checkboxes | ||
| .mdc-checkbox--disabled { |
There was a problem hiding this comment.
how is mdc-checkbox--disabled different from mdc-checkbox-background-filled-disabled and mdc-checkbox-background-unfilled-disabled?
There was a problem hiding this comment.
mdc-checkbox--disabled is a modifier class for CSS-only buttons that prevents them from displaying a ripple or pointer cursor on hover.
mdc-checkbox-unfilled-background-selector-disabled_ and friends are convenience mixins that wrap a block of Sass in the big ugly CSS selectors needed to target the 4 different state combinations. This will also hopefully help us keep the relatively complex selectors in sync by putting them all in one place.
| @@ -29,5 +29,5 @@ $mdc-checkbox-transition-duration: 90ms; | |||
| $mdc-checkbox-item-spacing: 4px; | |||
|
|
|||
| /* Manual calculation done on SVG */ | |||
There was a problem hiding this comment.
While you're in the neighborhood...switch this to a line comment to avoid emitting it
| background-color: $mdc-checkbox-mark-color; | ||
| opacity: 0; | ||
| @include mdc-checkbox-background-unfilled-disabled { | ||
| @include mdc-checkbox-stroke-color($mdc-checkbox-disabled-color); |
There was a problem hiding this comment.
I was about to say something like this :)
The nice thing about the button mixins is the user doesn't have to think about "enabled" at all, we apply it for them.
Also, we might be able to just use :enabled instead of :not(:disabled) here - the reason for :not(:disabled) on button was to also support its use for a elements across browsers.
Needed to correctly animate colors while transitioning between marked and unmarked states.
For clarity and consistency with other components
Use a single API instead of confusing clients by providing multiple similar-sounding mixins.
demos/checkbox.scss
Outdated
| @include mdc-checkbox-ripple((base-color: $color, opacity: $mdc-checkbox-ripple-opacity)); | ||
| @include mdc-checkbox-stroke-and-container-fill-color( | ||
| $unmarked-stroke-color: $color, | ||
| $unmarked-container-fill-color: rgba($color, .25), |
There was a problem hiding this comment.
Should this .25 be a public variable in mdc-checkbox?
There was a problem hiding this comment.
IMHO probably not - this is just a demo page, and we don't have any official material guidelines about what opacity to use for fill color of an unmarked checkbox.
demos/checkbox.scss
Outdated
| @include mdc-checkbox-ink-color(black); | ||
| @include mdc-checkbox-stroke-and-container-fill-color( | ||
| $unmarked-stroke-color: $color, | ||
| $unmarked-container-fill-color: rgba($color, .35), |
packages/mdc-checkbox/README.md
Outdated
|
|
||
| Mixin | Description | ||
| --- | --- | ||
| `mdc-checkbox-stroke-and-container-fill-color($unmarked-stroke-color, $unmarked-container-fill-color, $marked-stroke-color, $marked-container-fill-color, $animate)` | Generates CSS classes to set and animate the stroke color and/or container fill color of a checkbox |
There was a problem hiding this comment.
I hate using "and" in API names...
How about mdc-checkbox-marked-indicator?
There was a problem hiding this comment.
But it takes both marked and unmarked styles? Coming up with a good descriptive name for this is hard...
There was a problem hiding this comment.
Agreed, I'm not wild about the name either...
The mixin covers both marked and unmarked states, so putting marked in the name would probably be misleading. And I'm not sure I would call this an indicator, exactly.
Since stroke and fill are both properties of the container, what about:
mdc-checkbox-container-colors?
Making the mixin name plural will hopefully make it obvious that it takes multiple color arguments, and help differentiate it from the other theme mixins that only take one color argument.
There was a problem hiding this comment.
mdc-checkbox-container-colors sounds right to me.
packages/mdc-checkbox/README.md
Outdated
|
|
||
| All parameters are optional, and if left unspecified will use their default values. | ||
|
|
||
| If you plan to use CSS-only checkboxes, set `$animate` to `false` to prevent the mixin from generating `@keyframes` and CSS classes used by the JavaScript component. |
There was a problem hiding this comment.
I think it should be $keyframeAnimate...not just $animate. Doesn't the CSS-only checkbox still do some animation? Just not with keyframes?
There was a problem hiding this comment.
Getting super-pedantic here, but would that be transition rather than animation in that case? In which case animate might be enough distinction, but I could go either way.
There was a problem hiding this comment.
I originally named it $generate-keyframes, so I changed it back to that. Does that work?
You're correct - the CSS-only checkbox uses transition to tween from marked to unmarked states instead of animation. As you suggested, it's probably more accurate to name the parameter something with "keyframes", because that's what it really controls.
There was a problem hiding this comment.
generate-keyframes does sound better to me.
After discussing with design, it doesn't make sense to allow clients to customize stroke and fill color separately in the marked state. We still allow that in the unmarked state, however.
demos/checkbox.scss
Outdated
| $color: $material-color-blue-500; | ||
|
|
||
| @include mdc-checkbox-container-colors( | ||
| $unmarked-stroke-color: $color, |
There was a problem hiding this comment.
I would suggest making this color different to better illustrate how this mixin's parameters work now. I was seriously confused for several minutes thinking we were applying the same stroke color to both states, but IIUC that's not what we're doing; we're making the marked stroke color match the marked fill color, which makes sense.
packages/mdc-checkbox/README.md
Outdated
|
|
||
| #### `mdc-checkbox-container-colors($unmarked-stroke-color, $unmarked-fill-color, $marked-fill-color, $generate-keyframes)` | ||
|
|
||
| Generates CSS classes to set the stroke color and/or container fill color of a checkbox in its marked and unmarked states. In the unmarked state, stroke and fill color may be customized independently; in the marked state, the stroke disappears, and only the fill may be customized. |
There was a problem hiding this comment.
This sounds a little misleading... the stroke doesn't technically disappear, but it intentionally matches the marked fill color, they can't be customized separately.
[aria-disabled="true"]selector, as it doesn't appear to be necessaryResolves #1146