fix(slider): use secondary custom property color for slider container #5132
Conversation
packages/mdc-slider/_mixins.scss
Outdated
| @@ -435,8 +435,16 @@ | |||
| $feat-color: mdc-feature-create-target($query, color); | |||
|
|
|||
| .mdc-slider__track-container { | |||
| @include mdc-feature-targets($feat-color) { | |||
| @include mdc-theme-prop(background-color, rgba(mdc-theme-prop-value($color), $opacity)); | |||
| &::after { | |||
There was a problem hiding this comment.
This PR does not seem to change the color of slider to secondary color?
There was a problem hiding this comment.
Add the following to the test/screenshot/spec/mdc-slider/clases/baseline_continuous.html:
Inside of the head tag:
<style>
.test-secondary-color {
--mdc-theme-secondary: orange;
}
</style>
Inside of the body, add a second slider with the test-secondary-color class:
<div class="mdc-slider test-slider test-secondary-color" tabindex="0" role="slider"
aria-valuemin="0" aria-valuemax="100" aria-valuenow="0"
aria-label="Select Value">
<div class="mdc-slider__track-container">
<div class="mdc-slider__track"></div>
</div>
<div class="mdc-slider__thumb-container">
<svg class="mdc-slider__thumb" width="21" height="21">
<circle cx="10.5" cy="10.5" r="7.875"></circle>
</svg>
<div class="mdc-slider__focus-ring"></div>
</div>
</div>
Then run npm start and navigate to that page. You will see the added slider use the secondary color.
packages/mdc-slider/_mixins.scss
Outdated
| @@ -435,8 +435,16 @@ | |||
| $feat-color: mdc-feature-create-target($query, color); | |||
|
|
|||
| .mdc-slider__track-container { | |||
| @include mdc-feature-targets($feat-color) { | |||
| @include mdc-theme-prop(background-color, rgba(mdc-theme-prop-value($color), $opacity)); | |||
| &::after { | |||
There was a problem hiding this comment.
How is this using pseudo selector different from directly applying on the element?
There was a problem hiding this comment.
This comment on the issue over in material-components-web-components has more context: material-components/material-web#80 (comment)
Without the pseudo selector, the color of the slider track defaults to the primary color because it uses the mdc-theme-prop-value function which cannot handle secondary css variable definitions. Additionally, because rgba is used here previously, css variables will not function properly because passing in a css variable that is a hex value to an rgba function will invalid. The only way to make this work with css variables is by using a pseudo-class that fills the parent element and applying that css variable to the background-color and setting an opacity value. Applying an opacity value directly to the track container will also not work because that will then also apply to its children which means the track itself will be opaque which is not the desired state either.
svencowart
changed the title
|
@svencowart @abhiomkar is there any change this will get merged in the near future? It would fix a quite serious bug material-components/material-web#80 |
asyncLiz
changed the title
|
@svencowart can you rebase master onto this branch? # If you don't already have upstream set to this repo
git remote set-url upstream git@github.com:material-components/material-components-web.git
git pull --rebase upstream master
git push --force |
svencowart
force-pushed
the
slider-track-secondary
branch
from
2902240 to
f67f6d6
Compare
|
@asyncLiz I have rebased master onto this branch, but it seems the unit tests are not working because of some issue with starting Chrome. My permissions do not allow me to look into it further. |
|
Yea we're having some build issues, I'll look into it while I run internal tests |
| @@ -435,8 +435,16 @@ | |||
| $feat-color: feature-targeting-functions.create-target($query, color); | |||
|
|
|||
| .mdc-slider__track-container { | |||
| @include feature-targeting-mixins.targets($feat-color) { | |||
| @include theme-mixins.prop(background-color, rgba(theme-variables.prop-value($color), $opacity)); | |||
| @include feature-targeting-mixins.targets($feat-color) { | |||
|
@asyncLiz Unit tests does not work on forked repo. Let's merge this once the CL is submitted. Thanks! |
|
See cl/290088778 |
These changes fix the following issues which were introduced by material-components#5132: 1. The background of track is now provided by `.mdc-slider__track-container::after`, however `::after` wasn't positioned correctly which made it invisible. 2. The `::after` structural styles were set inside the `color` feature target. This means that they'll end up being included multiple times and in the wrong place.
These changes fix the following issues which were introduced by material-components#5132: 1. The background of track is now provided by `.mdc-slider__track-container::after`, however `::after` wasn't positioned correctly which made it invisible. 2. The `::after` structural styles were set inside the `color` feature target. This means that they'll end up being included multiple times and in the wrong place.
Overcomes css limitation of not being able to use a hex value inside of an rgba value by using the after pseudo class and applying opacity on it instead. In this way, the container and track are left alone and only the track container has the opacity applied to it.