feat(menu-surface): add feature targeting for styles #4279

crisbeto · 2019-01-21T21:57:00Z

Sets up style feature targeting for mdc-menu-surface.

mmalerba

I may try to take a stab at writing some kind of test or something to detect these nested mdc-feature-target mixins and complain about them, I think this would lead to unexpected results.

mmalerba · 2019-01-22T01:54:21Z

packages/mdc-menu-surface/_mixins.scss

+    @include mdc-feature-targets($feat-structure) {
+      @include mdc-rtl-reflexive-property(transform-origin, top left, top right);
+      @include mdc-menu-surface-shape-radius(medium);
+      @include mdc-menu-surface-base_($query);


move out of this $feat-structure block

mmalerba · 2019-01-22T01:55:22Z

packages/mdc-menu-surface/_mixins.scss

+  }
+
+  .mdc-menu-surface--anchor {
+    @include mdc-feature-targets($feat-structure) {


if you move this mdc-feature-targets up a level you can surround this rule and the one below

mmalerba · 2019-01-22T01:56:23Z

packages/mdc-menu-surface/_mixins.scss

+      transform: scale(.8);
+      opacity: 0;
+
+      @include mdc-feature-targets($feat-animation) {


move out of this $feat-structure block

mmalerba · 2019-01-22T01:57:02Z

packages/mdc-menu-surface/_mixins.scss

+      display: inline-block;
+      opacity: 0;
+
+      @include mdc-feature-targets($feat-animation) {


move out of this $feat-structure block

crisbeto · 2019-01-22T07:45:07Z

For what it's worth, I did try out the nested mdc-feature-target and it seemed to work as expected. That being said, after sleeping on it, it seems like a bad idea, because it means that you can't include the animation styles without including the structure.

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

Yeah I think it works in certain situations, but not others. If you want to sanity check your PRs, cherry pick in #4281 where I made it an error.

crisbeto · 2019-01-22T18:35:55Z

I've addressed the feedback @mmalerba.

mmalerba · 2019-01-22T23:37:48Z

The CSS diff shows that some rules changed order (possibly ok), but some also gained additional specificity which we probably don't want:

menu-surface-before.css
menu-surface-after.css

packages/mdc-menu-surface/_mixins.scss

crisbeto · 2019-01-25T20:20:17Z

@kfranqueiro I've rebased, fixed the redundant selectors and specificity changes.

kfranqueiro

LGTM now, thanks!

mmalerba · 2019-01-28T16:46:50Z

packages/mdc-menu-surface/_mixins.scss

+  }
+
+  // postcss-bem-linter: end
+}

 @mixin mdc-menu-surface-ink-color($color) {


Based on the discussion from the switch PR it sounds like we want to add $query support to all of the public mixins, that would mean these 3 too (and make sure to add them to the test file)

crisbeto · 2019-01-29T16:17:09Z

@mmalerba I've addressed the feedback.

kfranqueiro · 2019-02-01T19:15:22Z

packages/mdc-menu-surface/_mixins.scss

+    }
+  }
+
+  @include mdc-feature-targets($feat-structure) {


Invert these to put the feature-targets within each selector. Same for within base_.

kfranqueiro · 2019-02-01T19:17:31Z

test/scss/feature-targeting.scss

@@ -53,4 +54,9 @@
  @include mdc-typography-overflow-ellipsis($query: mdc-feature-any());
  @include mdc-typography-baseline-top(0, $query: mdc-feature-any());
  @include mdc-typography-baseline-bottom(0, $query: mdc-feature-any());
+
+  // Menu surface


This section is missing a call to the baseline mdc-menu-surface mixin. Also can we insert this after menu rather than at the end?

kfranqueiro · 2019-02-01T19:31:15Z

packages/mdc-menu-surface/_mixins.scss

+  .mdc-menu-surface {
+    @include mdc-menu-surface-base_($query);
+    @include mdc-elevation($z-value: 8, $query: $query);
+


Remove empty line (fails lint)

Sets up style feature targeting for `mdc-menu-surface`.

crisbeto · 2019-02-02T11:50:54Z

@kfranqueiro the latest set of feedback is addressed.

kfranqueiro · 2019-02-04T15:42:36Z

Diff looks fine; I ran the mdc-menu screenshot tests and no changes were found. https://storage.googleapis.com/mdc-web-screenshot-tests/kfranqueiro/2019/02/04/15_38_21_044/report/report.html

googlebot added the cla: yes label Jan 21, 2019

crisbeto force-pushed the menu-surface-style-targeting branch from aed775f to 9e1bc4e Compare January 21, 2019 22:03

mmalerba suggested changes Jan 22, 2019

View reviewed changes

crisbeto force-pushed the menu-surface-style-targeting branch from 9e1bc4e to 12a162a Compare January 22, 2019 18:35

kfranqueiro mentioned this pull request Jan 24, 2019

Allow finer-grained control of emitted CSS #4227

Closed

35 tasks

kfranqueiro suggested changes Jan 24, 2019

View reviewed changes

packages/mdc-menu-surface/_mixins.scss Outdated Show resolved Hide resolved

packages/mdc-menu-surface/_mixins.scss Outdated Show resolved Hide resolved

crisbeto force-pushed the menu-surface-style-targeting branch from 12a162a to 39ddcf1 Compare January 25, 2019 20:19

crisbeto force-pushed the menu-surface-style-targeting branch from 39ddcf1 to aa3b943 Compare January 26, 2019 15:03

kfranqueiro approved these changes Jan 28, 2019

View reviewed changes

mmalerba suggested changes Jan 28, 2019

View reviewed changes

crisbeto force-pushed the menu-surface-style-targeting branch from 5a93a03 to 04f9171 Compare January 31, 2019 21:54

kfranqueiro reviewed Feb 1, 2019

View reviewed changes

kfranqueiro suggested changes Feb 1, 2019

View reviewed changes

kfranqueiro reviewed Feb 1, 2019

View reviewed changes

crisbeto and others added 4 commits February 2, 2019 12:44

feat(menu-surface): add feature targeting for styles

2b96fc4

Sets up style feature targeting for `mdc-menu-surface`.

Fix lint errors

4811539

refactor: address feedback

c1dc24b

refactor: apply latest style API changes

16f6e97

crisbeto force-pushed the menu-surface-style-targeting branch from 04f9171 to 1239b9f Compare February 2, 2019 11:50

refactor: address more feedback

c1e3272

crisbeto force-pushed the menu-surface-style-targeting branch from 1239b9f to c1e3272 Compare February 2, 2019 11:59

kfranqueiro approved these changes Feb 4, 2019

View reviewed changes

kfranqueiro merged commit 56b8467 into material-components:master Feb 4, 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(menu-surface): add feature targeting for styles #4279

feat(menu-surface): add feature targeting for styles #4279

crisbeto commented Jan 21, 2019

mmalerba left a comment

mmalerba Jan 22, 2019

mmalerba Jan 22, 2019

mmalerba Jan 22, 2019

mmalerba Jan 22, 2019

crisbeto commented Jan 22, 2019

mmalerba commented Jan 22, 2019

crisbeto commented Jan 22, 2019

mmalerba commented Jan 22, 2019

crisbeto commented Jan 25, 2019 •

edited

Loading

kfranqueiro left a comment

mmalerba Jan 28, 2019

crisbeto commented Jan 29, 2019

kfranqueiro Feb 1, 2019

kfranqueiro Feb 1, 2019

kfranqueiro Feb 1, 2019

crisbeto commented Feb 2, 2019

kfranqueiro commented Feb 4, 2019

feat(menu-surface): add feature targeting for styles #4279

feat(menu-surface): add feature targeting for styles #4279

Conversation

crisbeto commented Jan 21, 2019

mmalerba 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 22, 2019

mmalerba commented Jan 22, 2019

crisbeto commented Jan 22, 2019

mmalerba commented Jan 22, 2019

crisbeto commented Jan 25, 2019 • edited Loading

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Feb 2, 2019

kfranqueiro commented Feb 4, 2019

crisbeto commented Jan 25, 2019 •

edited

Loading