feat(fab): Add feature targeting for styles #4526
Conversation
|
All 630 screenshot tests passed for commit 68cb08e vs. |
packages/mdc-fab/_mixins.scss
Outdated
| } @else { | ||
| @include mdc-fab-ink-color(text-primary-on-light); | ||
| @include mdc-states(text-primary-on-light); | ||
| @include mdc-feature-targets($feat-color) { |
There was a problem hiding this comment.
I think you can remove this one since you're passing the query through below
There was a problem hiding this comment.
Good point... I had originally forgotten to forward the query through the includes below, and then forgot about the include here.
...Meanwhile, now I'm spooked because this somehow didn't fail building? Seems like it should have...
There was a problem hiding this comment.
Yeah that is strange, I can see how the feature-targeting test would miss it. We gave it a query that selects nothing, so it fails the first feature-targets check and Sass doesn't bother evaluating what's inside.
But is this getting built as part of the dev app?
material-components-web/demos/fab.scss
Line 93 in f310e73
There was a problem hiding this comment.
Oh actually it did fail when I checked out your PR, reverted the fix commit and tried to npm run dev. Got the following error:
ERROR in ./demos/fab.scss
Module build failed:
@error "mdc-feature-targets must not be used inside of another mdc-feature-targets block";
I guess maybe this isn't built as part of your CI?
There was a problem hiding this comment.
I made a PR (#4528) to run all the test mixins twice, once with none of the features and once with all of them. In the all of them case, we don't need to do anything with the output, just make sure that it compiles. I think this would have caught your issue here
|
All 630 screenshot tests passed for commit 1e89a55 vs. |
|
All 630 screenshot tests passed for commit 33dd8ab vs. |
|
All 630 screenshot tests passed for commit 54e4966 vs. |
(cherry picked from commit 1ba7bdd)
Refs #4227.
There are CSS diffs due to reordering, but the same styles should still be present.
cc @mmalerba