Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(floating-label): add feature targeting for styles #5287

Conversation

crisbeto
Copy link
Collaborator

Adds support for feature targeting to the mdc-floating-label styles.

Relates to #4227.

@codecov-io
Copy link

codecov-io commented Nov 28, 2019

Codecov Report

Merging #5287 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5287      +/-   ##
==========================================
- Coverage   98.52%   98.49%   -0.04%     
==========================================
  Files         163      163              
  Lines        6313     6313              
  Branches      865      787      -78     
==========================================
- Hits         6220     6218       -2     
- Misses         93       95       +2
Impacted Files Coverage Δ
testing/dom/events.ts 71.42% <0%> (-28.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ffe8f7...59a6189. Read the comment docs.

@@ -159,6 +160,15 @@
@include mdc-elevation-overlay-fill-color(red, $query: $query);
@include mdc-elevation-overlay-opacity(99%, $query: $query);

// Floating label
@include mdc-floating-label-core-styles($query: $query);
Copy link
Collaborator Author

@crisbeto crisbeto Nov 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the test:feature-targeting check doesn't seem to be running on the CI because there are failures in the file that's in master. I've added my changes here for completeness.

@crisbeto crisbeto marked this pull request as ready for review November 28, 2019 20:21
@abhiomkar abhiomkar requested a review from mmalerba December 6, 2019 16:36
Copy link
Collaborator

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can you provide a diff of the emitted CSS before and after to verify no unexpected changes?

packages/mdc-floating-label/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-floating-label/_mixins.scss Outdated Show resolved Hide resolved
packages/mdc-floating-label/mdc-floating-label.scss Outdated Show resolved Hide resolved
@crisbeto
Copy link
Collaborator Author

crisbeto commented Dec 6, 2019

I've addressed the feedback @mmalerba and I've attached the before/after files. There are a few lines of difference, but they're within the same rule and shouldn't have an effect at runtime. I moved these lines down so that they could fit into an existing feature target.

before.css.txt
after.css.txt

@crisbeto crisbeto force-pushed the floating-label-feature-targeting branch from 2a96876 to 4be58be Compare December 6, 2019 18:31
}
// postcss-bem-linter: define floating-label
.mdc-floating-label {
@include mdc-typography(subtitle1, $query: $query);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Please use $exclude-props to exclude line-height in typography to avoid duplication with below CSS override.

BUILD will throw an error otherwise. :)

/* @alternative */ comment before CSS override would also fix this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

Adds support for feature targeting to the `mdc-floating-label` styles.

Relates to material-components#4227.
@crisbeto crisbeto force-pushed the floating-label-feature-targeting branch from 4be58be to 59a6189 Compare December 18, 2019 05:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants