-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(checkbox): add feature targeting for styles #4258
feat(checkbox): add feature targeting for styles #4258
Conversation
FYI: @kfranqueiro |
Codecov Report
@@ Coverage Diff @@
## master #4258 +/- ##
==========================================
+ Coverage 98.58% 98.91% +0.33%
==========================================
Files 127 127
Lines 5705 5705
Branches 762 762
==========================================
+ Hits 5624 5643 +19
+ Misses 81 62 -19
Continue to review full report at Codecov.
|
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.
Thanks for sending this out so quick! I saw what you meant RE the potential for reordering a few styles to combine blocks and those mostly seem reasonable. I left comments on the ones I saw, along with a few other things.
@include mdc-ripple-radius-unbounded; | ||
} | ||
|
||
@at-root { |
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.
I should point out that one change I did make was to remove this @at-root
. It didn't seem necessary, and it would prevent people from doing something like:
@include mdc-checkbox();
.my-app-alternate-theme {
// ... change some theme variables ...
@include mdc-checkbox(color);
}
Correct me if I'm wrong though.
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.
Yeah, I noticed this, and realized that it seemed unnecessary, since it wasn't nested under anything and afaik @include
can be done top-level anyway. We might've gotten a little carried away when we were trying to reduce specificity.
327d4b6
to
5bf9d98
Compare
854ea7e
to
54570c1
Compare
54570c1
to
9d2d395
Compare
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, diffed before/after and the only diffs are moved properties which look fair. Currently running the mdc-checkbox screenshot tests locally to confirm no diffs, since they can't run on PRs from forks.
Oh interesting about the fork thing, should I be writing code on branches instead? |
I presume we'd have to give you access to do that, right? I don't think it's too big of a deal in these cases to run a subset of our screenshot tests manually. Here's the report with 0 diffs for the checkbox screenshot tests: https://storage.googleapis.com/mdc-web-screenshot-tests/kfranqueiro/2019/01/24/21_26_52_748/report/report.html?utm_source=cli&utm_medium=test_results |
This PR adds Sass feature targeting to the checkbox component as part of #4227.
Currently blocked on #4228 which adds the
mdc-feature-targeting
package.