feat: Add feature targeting support and apply to mdc-button #4228
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4228 +/- ##
=========================================
- Coverage 98.87% 98.58% -0.3%
=========================================
Files 127 127
Lines 5705 5705
Branches 762 762
=========================================
- Hits 5641 5624 -17
- Misses 64 81 +17
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
First pass...I think I have a pretty good understanding, but it took me a bit to understand the new feature-targeting package. The cyclomatic-complexity is quite high, which takes some time to trace. Its a cool system with a lot of bells and whistles (so I see why we went this route).
There was a problem hiding this comment.
It took me a while to completely understand what's happening just by looking at the code.
I like the idea of allowing fine grained control of emitting CSS based on target features. This also brings lot of complexity into the system. I echo Matt's point here.
I've only looked at the mdc-button/ and mdc-feature-targeting/ packages & I've few questions / suggestions.
Can we group the features into a separate mixins and use each feature as al-carte?
For example,
.mdc-button {
@include mdc-button-common;
@include mdc-button-feature-all;
}
@mixin mdc-button-feature-all {
// These mixins can be used individually only if needed.
@include mdc-button-feature-typography;
@include mdc-button-feature-color;
@include mdc-button-feature-structure;
}
@mixin mdc-button-common {
@include mdc-ripple-surface;
@include mdc-ripple-radius-bounded;
@include mdc-states(primary);
...
}
@mixin mdc-button-feature-typography {
@include mdc-typography(button);
}
@mixin mdc-button-feature-color {
@include mdc-button-ink-color(primary);
.mdc-button--raised,
.mdc-button--unelevated {
@include mdc-button-container-fill-color(primary);
@include mdc-button-ink-color(on-primary);
}
}
@mixin mdc-button-feature-structure {
@include mdc-button-shape-radius(small);
...
}
I'm sure you might've thought about this, was there any challenges with this approach? :)
Also, It would be great to see how much of CSS (or bytes) we're saving before & after this change.
|
Regarding CSS file size before/after, the difference should ideally be zero. The reason this is structured the way it is is to be able to easily verify that we're outputting the same styles before and after these changes. I wonder if your other idea RE organization occurred to @mmalerba (I suspect he was struck with a few ideas as he prototyped this). The main drawback I can think of for the purposes of the conversion itself is that it'd make it harder to glance at the generated CSS before/after to confirm they're the same, because it would likely reorder a bunch of styles (in a way that is harmless, but would require a lot more careful inspection of diffs, especially once we get into e.g. text field). I think @mmalerba purposely aimed for keeping the output CSS identical to before. I definitely agree that the feature-targeting package can be hard to grok at first - and that makes me realize it doesn't yet have a README here. I'd like to get this in first so that people can start pushing forward with more conversions (the Angular team is interested in helping out), but I'm happy to take on writing the README (perhaps with a review from Miles) separately. |
|
@abhiomkar Yes, I did consider just having separate mixins for each feature, but I felt their were a number of advantages this approach had over it:
|
|
All 758 screenshot tests passed for commit 22d6e7d vs. |
| // | ||
|
|
||
| // Creates a feature target from the given feature query and targeted feature. | ||
| @function mdc-feature-create-target($feature-query, $targeted-feature) { |
There was a problem hiding this comment.
It would be nice to have examples in comment line similar to https://github.com/material-components/material-components-web/blob/master/packages/mdc-shape/_functions.scss#L28
There was a problem hiding this comment.
I'm inclined to tackle this along with the README for the feature-targeting package, in a follow-up PR.
(Edit: Logged #4266)
|
All 758 screenshot tests passed for commit 9d7bf83 vs. |
| background-color $mdc-states-wash-duration linear; | ||
| } | ||
|
|
||
| @include mdc-feature-targets($feat-structure) { |
There was a problem hiding this comment.
I think it is safe to merge these structure style to above feat-structure block.
Comment applies across this file.
There was a problem hiding this comment.
I gave this a try in a few places, but I'm ultimately deciding against it for 2 reasons:
-
There are different selectors in the block above vs. here, whereas this block currently incorporates structure and animation styles for the same selector. I'd argue this is easier to read, because it's closer to how you would read raw CSS prior to the feature-targeting split (it's just a set of properties under the same selector).
-
Moving this to the other selector causes more output CSS due to needing to duplicate selectors. That effect is then multiplied by the number of components that
@includeripple mixins.
In contrast, when there are cases of multiple disjointed mdc-feature-targets calls on the same feature under the same exact selector, those are much more likely to make sense to merge together (Miles and I identified a few on his checkbox PR #4258, for example). But this case is more complex.
I did make a couple of tweaks here and in button to attempt to make more selectors easier to spot, by moving selectors outside feature-targets invocations (again taking advantage of Sass being smart enough to prune empty blocks).
|
All 758 screenshot tests passed for commit 9677b66 vs. |
This adds a
mdc-feature-targetingpackage to support the effort summarized in #4227, and converts the mdc-button package (making necessary adjustments to mdc-ripple to allow it).Many thanks to @mmalerba for spearheading the initial design and implementation.
The most important thing to note about the feature-targeting infrastructure is that it should be used as little as necessary to provide the granularity outlined in #4227 - i.e.,
$queryis only passed down as far as it needs to be passed to ensure this granularity. It is not passed to every single mixin.This PR also adds a new test script to verify that nothing is missed when feature-targeting. All this requires to work is that as we restructure each package's SCSS, we add an import for it in
test/scss/feature-targeting.scss.