chore(list): remove dark theme #2082
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2082 +/- ##
=======================================
Coverage 99.33% 99.33%
=======================================
Files 84 84
Lines 3769 3769
Branches 490 490
=======================================
Hits 3744 3744
Misses 25 25Continue to review full report at Codecov.
|
packages/mdc-list/_variables.scss
Outdated
| @@ -13,7 +13,6 @@ | |||
| // limitations under the License. | |||
|
|
|||
| $mdc-list-divider-color-light: rgba(0, 0, 0, .12) !default; | |||
| $mdc-list-divider-color-dark: rgba(255, 255, 255, .2) !default; | |||
There was a problem hiding this comment.
I'm wondering one of two things here...
- Should we keep this variable and use
mdc-theme-contrast-tonein our styles to determine whether to use light or dark based on the value of the background color? - Should we remove this and also remove
-lightfrom the other variable?
cc @acdvorak
There was a problem hiding this comment.
So we would just keep this style in here that other people can reference if they create their own "dark-theme"?
There was a problem hiding this comment.
I vote for option 1: keep this variable and use mdc-theme-contrast-tone.
E.g.:
@at-root {
$divider-color: if(
mdc-theme-contrast-tone($mdc-theme-background) == "dark",
$mdc-list-divider-color-dark,
$mdc-list-divider-color-light
);
@include mdc-list-divider-color($divider-color);
}That way, if the client overrides $mdc-theme-background, we will automatically set the correct divider color for them. If they don't like the color we choose, they can always override it with the mixin.
There was a problem hiding this comment.
Ok I see what you guys are saying now. I will update as stated here. Thanks!
| @@ -21,26 +21,12 @@ | |||
| @import "./mixins"; | |||
| @import "./variables"; | |||
|
|
|||
| .mdc-list, | |||
There was a problem hiding this comment.
I'm pretty sure you can remove the import for @material/theme/mixins after removing the dark styles in this file? (We also missed this for mdc-button.scss I think, so might want to send a tiny separate PR for that, and check on the other open PRs)
There was a problem hiding this comment.
Will do...good call.
…ponents/material-components-web into feat/list/remove-dark-theme
…ponents/material-components-web into feat/list/remove-dark-theme
kfranqueiro
dismissed
their stale review
Comment was addressed, Andy is taking this over
packages/mdc-list/_variables.scss
Outdated
| @@ -14,6 +14,5 @@ | |||
|
|
|||
| $mdc-list-divider-color-light: rgba(0, 0, 0, .12) !default; | |||
There was a problem hiding this comment.
The names of these variables are confusing - can we rename them while we're here? E.g.:
$mdc-list-divider-color-on-light-bg: rgba(0, 0, 0, .12) !default;
$mdc-list-divider-color-on-dark-bg: rgba(255, 255, 255, .2) !default;We'll need to add BREAKING CHANGE to the PR description (and the commit message when we squash and merge).
packages/mdc-list/mdc-list.scss
Outdated
| @@ -191,7 +170,13 @@ a.mdc-list-item { | |||
| } | |||
|
|
|||
| @at-root { | |||
| @include mdc-list-divider-color($mdc-list-divider-color-light); | |||
| $divider-color: if( | |||
| mdc-theme-contrast-tone($mdc-theme-background) == "dark", | |||
There was a problem hiding this comment.
My mistake - this should actually be mdc-theme-tone($mdc-theme-background) == "dark"
The $mdc-list-divider-color-[light|dark] variables are named confusingly, so I got them mixed up.
If you look at the "Full-Width Dividers" and "Inset Dividers" examples, the dividers should be visible; currently they're not (because the colors were flipped).
There was a problem hiding this comment.
yup just noticed...looks better
…ponents/material-components-web into feat/list/remove-dark-theme
BREAKING CHANGE: renamed divider scss vars `$mdc-list-divider-color-light` to `$mdc-list-divider-color-on-light-bg`, `$mdc-list-divider-color-dark` to ``$mdc-list-divider-color-on-dark-bg`
No description provided.