feat(menu-surface): add feature targeting for styles #4279
Conversation
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
aed775f
to
9e1bc4e
Compare
| @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); |
There was a problem hiding this comment.
move out of this $feat-structure block
| } | ||
|
|
||
| .mdc-menu-surface--anchor { | ||
| @include mdc-feature-targets($feat-structure) { |
There was a problem hiding this comment.
if you move this mdc-feature-targets up a level you can surround this rule and the one below
| transform: scale(.8); | ||
| opacity: 0; | ||
|
|
||
| @include mdc-feature-targets($feat-animation) { |
There was a problem hiding this comment.
move out of this $feat-structure block
| display: inline-block; | ||
| opacity: 0; | ||
|
|
||
| @include mdc-feature-targets($feat-animation) { |
There was a problem hiding this comment.
move out of this $feat-structure block
|
For what it's worth, I did try out the nested |
|
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
force-pushed
the
menu-surface-style-targeting
branch
from
9e1bc4e
to
12a162a
Compare
|
I've addressed the feedback @mmalerba. |
|
The CSS diff shows that some rules changed order (possibly ok), but some also gained additional specificity which we probably don't want: |
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
12a162a
to
39ddcf1
Compare
|
@kfranqueiro I've rebased, fixed the redundant selectors and specificity changes. |
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
39ddcf1
to
aa3b943
Compare
| } | ||
|
|
||
| // postcss-bem-linter: end | ||
| } | ||
|
|
||
| @mixin mdc-menu-surface-ink-color($color) { |
There was a problem hiding this comment.
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)
|
@mmalerba I've addressed the feedback. |
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
5a93a03
to
04f9171
Compare
| } | ||
| } | ||
|
|
||
| @include mdc-feature-targets($feat-structure) { |
There was a problem hiding this comment.
Invert these to put the feature-targets within each selector. Same for within base_.
test/scss/feature-targeting.scss
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
| .mdc-menu-surface { | ||
| @include mdc-menu-surface-base_($query); | ||
| @include mdc-elevation($z-value: 8, $query: $query); | ||
|
|
There was a problem hiding this comment.
Remove empty line (fails lint)
Sets up style feature targeting for `mdc-menu-surface`.
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
04f9171
to
1239b9f
Compare
|
@kfranqueiro the latest set of feedback is addressed. |
crisbeto
force-pushed
the
menu-surface-style-targeting
branch
from
1239b9f
to
c1e3272
Compare
|
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 |
Sets up style feature targeting for
mdc-menu-surface.