Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants