-
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(feature-targeting): target top-level styles for elevation, ripple, #4383
feat(feature-targeting): target top-level styles for elevation, ripple, #4383
Conversation
packages/mdc-elevation/_mixins.scss
Outdated
@@ -25,6 +25,23 @@ | |||
@import "@material/theme/variables"; | |||
@import "./variables"; | |||
|
|||
@mixin mdc-elevation-top-level($query: mdc-feature-all()) { |
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.
For a couple of them I had to do mdc-<package>-top-level
since mdc-<package>
was taken. Should I go back and change them all to match for consistency?
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.
That's a fair point; it'd probably be less confusing that way (esp. for stuff like mdc-theme).
Also, do you think mdc-foo-baseline
would work for naming rather than mdc-foo-top-level
?
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.
That would work, though in the case of typography people might think it has something to do with baseline alignment
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.
per slcak discussion, going with mdc-foo-core-styles
cc61488
to
01513a6
Compare
packages/mdc-elevation/_mixins.scss
Outdated
@@ -25,6 +25,23 @@ | |||
@import "@material/theme/variables"; | |||
@import "./variables"; | |||
|
|||
@mixin mdc-elevation-top-level($query: mdc-feature-all()) { |
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.
That's a fair point; it'd probably be less confusing that way (esp. for stuff like mdc-theme).
Also, do you think mdc-foo-baseline
would work for naming rather than mdc-foo-top-level
?
$feat-color: mdc-feature-create-target($query, color); | ||
|
||
:root { | ||
@include mdc-feature-targets($feat-color) { |
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.
Is there a reason this feature-targets is outside the @each
but the ones below are inside?
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.
This one doesn't have any selectors inside of it, just properties
01513a6
to
7e4fe85
Compare
7e4fe85
to
2f1d32c
Compare
Codecov Report
@@ Coverage Diff @@
## master #4383 +/- ##
=========================================
+ Coverage 98.56% 98.9% +0.33%
=========================================
Files 130 130
Lines 5731 5731
Branches 763 763
=========================================
+ Hits 5649 5668 +19
+ Misses 82 63 -19
Continue to review full report at Codecov.
|
theme, and typography (part of #4227)