-
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(grid-list): Add feature targeting for styles #4534
Conversation
All 630 screenshot tests passed for commit d7ef4ad vs. |
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.
LGTM. left a comment to update package.json
line-height: 1rem; | ||
} | ||
|
||
@include mdc-feature-targets($feat-typography) { |
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.
Curious what does the typography includes? Should font-size and line-height be part of typography?
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.
It's primarily intended to include things that depend on the mdc-typography scale variables. Thinking on this particular line again (which @mmalerba brought up), I'm not even sure it's absolutely necessary for this, since this is just font-weight... Thoughts, Miles? What's the rule of thumb for things falling under the typography feature aside from direct uses of the mdc-typography
mixin?
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.
Like you said, I think if it depends on variables that we want the user to configure then it should be in the typography feature. That way users can do something like this:
@include mdc-grid-list-core-styles(mdc-without(typography));
.normal-typography-section {
@include mdc-grid-list-core-styles(typography);
}
.fancy-typography-section {
$mdc-typography-font-weight-values: $fancy-typography-weights !global;
@include mdc-grid-list-core-styles(typography);
}
Basically when they call the mixin with the typography
feature as the query, they're asking "give me all of the style rules that might have changed as a result of my messing with the typography variables"
Codecov Report
@@ Coverage Diff @@
## master #4534 +/- ##
=======================================
Coverage 99.02% 99.02%
=======================================
Files 130 130
Lines 6279 6279
Branches 820 820
=======================================
Hits 6218 6218
Misses 60 60
Partials 1 1 Continue to review full report at Codecov.
|
All 630 screenshot tests passed for commit 479fe98 vs. |
All 630 screenshot tests passed for commit b3418bf vs. |
Refs #4227.
cc @mmalerba