feat(switch): add feature targeting for styles #4275

crisbeto · 2019-01-19T15:32:33Z

Sets up style feature targeting for the switch component.

mmalerba · 2019-01-19T17:09:06Z

packages/mdc-switch/_mixins.scss

@@ -21,10 +21,89 @@
 //

 @import "@material/theme/mixins";
+@import "@material/ripple/common";


This actually emits a bunch of styles, instead you should @include the mixin version:

@mixin mdc-switch(...) { $feat-animation: ... $feat-color: ... $feat-structure: ... @include mdc-ripple-common($query); ... }

You can add an entry for mdc-switch here: https://github.com/material-components/material-components-web/blob/master/test/scss/feature-targeting.scss and then run npm run test:feature-targeting to ensure that you've targeted all the styles

mmalerba · 2019-01-19T17:15:48Z

packages/mdc-switch/_mixins.scss

+      @include mdc-switch-toggled-on-thumb-color($mdc-switch-baseline-theme-color);
+      @include mdc-switch-toggled-off-track-color($mdc-switch-toggled-off-track-color);
+      @include mdc-switch-toggled-off-thumb-color($mdc-switch-toggled-off-thumb-color);
+      @include mdc-switch-toggled-off-ripple-color($mdc-switch-toggled-off-ripple-color);


mdc-states can take a $query, so pass it through here and then pass it through to mdc-states

mmalerba · 2019-01-19T17:19:26Z

packages/mdc-switch/_mixins.scss

+
+  @include mdc-feature-targets($feat-structure) {
+    @include mdc-rtl-reflexive-position(left, $mdc-switch-tap-target-initial-position);
+    @include mdc-ripple-surface();


mdc-ripple-surface takes a $query

mmalerba · 2019-01-19T17:22:40Z

packages/mdc-switch/_mixins.scss

+  }
+}
+
+@mixin mdc-switch__native-control_($query: mdc-feature-all()) {


since this whole mixin is structure, there's no need to pass in $query, just wrap the @include

mmalerba · 2019-01-19T17:26:45Z

packages/mdc-switch/_mixins.scss

+  $feat-structure: mdc-feature-create-target($query, structure);
+
+  @include mdc-feature-targets($feat-structure) {
+    @include mdc-elevation(2);


mdc-elevation should actually be under color, since users can customize the shadow color

Replying to this comment to note this hasn't been fixed yet.

crisbeto · 2019-01-20T08:23:16Z

Updated based on the feedback from @mmalerba.

mmalerba · 2019-01-22T01:34:12Z

packages/mdc-switch/_mixins.scss

+  // postcss-bem-linter: end
+}
+
+@mixin mdc-switch-toggled-on-color($color, $query: mdc-feature-all()) {


@kfranqueiro should we add feature targeting to only the main public mixin, or these ones too? (If we are adding it here too, we should add these mixins to the test that verifies nothing is emitted for empty query, I think it would fail currently)

That's a good point, this mixin is not invoked by the base styles, so we don't necessarily need to cover it, if the goal is specifically to add feature targeting to the base emitted styles. If you're fine with that, then I think we can omit $query from mdc-switch-toggled-on-color and mdc-switch-toggled-off-color.

I could also see doing it, just for the sake of consistency on public mixins. If we always do it then people don't have to think about whether this is one of the ones that supports it.

Also for things like disabling animations, you probably do want to have the ability to exclude the animation feature from all mixins, not just the base one.

If we're parameterizing $query here, then I think we need to enclose the first 2 mixin invocations within a color feature-targets block (same for mdc-switch-toggled-off-color), and invoke these in test/scss/feature-targeting.scss as Miles suggested.

mmalerba · 2019-01-22T18:04:17Z

It looks like the order of some of the rules got swapped around. I'm not sure if it matters or not (potentially could due to specificity reasons). Here's the before and after for diffing:

switch-before.css
switch-after.css

crisbeto · 2019-01-22T18:19:40Z

I don't think there's much overlap between the different style declarations so we shouldn't run into any issues with ordering.

crisbeto · 2019-01-25T20:06:44Z

@kfranqueiro I've rebased the PR and double-checked the output changes. It seems like a couple of unrelated styles got swapped which didn't have a visual effect.

crisbeto · 2019-01-26T15:17:50Z

Looks like the changes that catch nested feature target mixins caught a couple in this PR. I moved a couple of styles around to fix the errors.

kfranqueiro

I made a few suggestions, including some that make the diff easier to look at, but it looks like all the styles are intact.

kfranqueiro · 2019-01-28T16:06:42Z

packages/mdc-switch/_mixins.scss

+  // postcss-bem-linter: end
+}
+
+@mixin mdc-switch-toggled-on-color($color, $query: mdc-feature-all()) {


If we're parameterizing $query here, then I think we need to enclose the first 2 mixin invocations within a color feature-targets block (same for mdc-switch-toggled-off-color), and invoke these in test/scss/feature-targeting.scss as Miles suggested.

kfranqueiro · 2019-01-28T16:20:08Z

packages/mdc-switch/_mixins.scss

+  $feat-color: mdc-feature-create-target($query, color);
+  $feat-structure: mdc-feature-create-target($query, structure);
+
+  @include mdc-states($mdc-switch-baseline-theme-color, false, $query);


Move mdc-states after mdc-ripple-surface (mdc-ripple-surface first is a common pattern across the packages that use it)

kfranqueiro · 2019-01-28T16:23:12Z

packages/mdc-switch/_mixins.scss

+  @include mdc-ripple-common($query);
+
+  .mdc-switch {
+    @include mdc-switch-toggled-off-ripple-color($mdc-switch-toggled-off-ripple-color, $query);


Move this to the end of this block to keep it near the other related color styles (and reduce diff)

kfranqueiro · 2019-01-28T16:24:14Z

packages/mdc-switch/_mixins.scss

+    @include mdc-switch__thumb-underlay_($query);
+  }
+
+  .mdc-switch__native-control {


Move this above .mdc-switch__track to reduce diff

crisbeto · 2019-01-29T16:11:49Z

@kfranqueiro I've addressed the latest set of feedback. Can you take a look at feature-targeting.scss in particular? I'm not sure that I completely understood what you meant with your comment.

Sets up style feature targeting for the switch component.

kfranqueiro · 2019-02-01T18:43:58Z

The update to feature-targeting.scss looked fine except that it also needed to invoke the baseline mdc-switch mixin to test feature targeting in that. I added that and fixed lint issues in the mixins (check with npm run lint:css locally to catch them early).

packages/mdc-switch/_mixins.scss

googlebot added the cla: yes label Jan 19, 2019

mmalerba requested changes Jan 19, 2019

View reviewed changes

crisbeto force-pushed the switch-targeting branch from 08f8f7f to 7d64bdc Compare January 20, 2019 08:24

mmalerba requested changes Jan 22, 2019

View reviewed changes

kfranqueiro mentioned this pull request Jan 24, 2019

Allow finer-grained control of emitted CSS #4227

Open

35 tasks

crisbeto force-pushed the switch-targeting branch from 7d64bdc to f40bcc3 Compare January 25, 2019 20:05

crisbeto force-pushed the switch-targeting branch from f40bcc3 to c4a4751 Compare January 26, 2019 15:14

kfranqueiro suggested changes Jan 28, 2019

View reviewed changes

crisbeto force-pushed the switch-targeting branch from c4a4751 to ed2bbf5 Compare January 29, 2019 16:10

crisbeto force-pushed the switch-targeting branch from ed2bbf5 to bbc93a2 Compare January 29, 2019 16:17

crisbeto added 3 commits January 31, 2019 22:21

feat(switch): add feature targeting for styles

b8d79f3

Sets up style feature targeting for the switch component.

refactor: address feedback

b937c42

refactor: apply latest style API changes

5f6ad5f

crisbeto force-pushed the switch-targeting branch from bbc93a2 to 5f6ad5f Compare January 31, 2019 21:44

kfranqueiro added 3 commits February 1, 2019 13:40

WIP Fix lint

92d05f9

WIP Add baseline switch mixin to test

ef39443

WIP Move radius-unbounded above states

ebb33ba

Merge branch 'master' into switch-targeting

acdb82b

kfranqueiro approved these changes Feb 1, 2019

View reviewed changes

packages/mdc-switch/_mixins.scss Show resolved Hide resolved

kfranqueiro merged commit 4836293 into material-components:master Feb 1, 2019

jamesmfriedman mentioned this pull request Feb 5, 2019

Upgrade to MDC 0.44.0 rmwc/rmwc#404

Closed

24 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(switch): add feature targeting for styles #4275

feat(switch): add feature targeting for styles #4275

crisbeto commented Jan 19, 2019

mmalerba Jan 19, 2019

mmalerba Jan 19, 2019

mmalerba Jan 19, 2019

mmalerba Jan 19, 2019

mmalerba Jan 19, 2019

kfranqueiro Jan 25, 2019

crisbeto commented Jan 20, 2019

mmalerba Jan 22, 2019

kfranqueiro Jan 25, 2019

mmalerba Jan 25, 2019

kfranqueiro Jan 28, 2019

mmalerba commented Jan 22, 2019

crisbeto commented Jan 22, 2019

crisbeto commented Jan 25, 2019

crisbeto commented Jan 26, 2019

kfranqueiro left a comment

kfranqueiro Jan 28, 2019

kfranqueiro Jan 28, 2019

kfranqueiro Jan 28, 2019

kfranqueiro Jan 28, 2019

crisbeto commented Jan 29, 2019

kfranqueiro commented Feb 1, 2019

feat(switch): add feature targeting for styles #4275

feat(switch): add feature targeting for styles #4275

Conversation

crisbeto commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jan 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Jan 22, 2019

crisbeto commented Jan 22, 2019

crisbeto commented Jan 25, 2019

crisbeto commented Jan 26, 2019

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jan 29, 2019

kfranqueiro commented Feb 1, 2019