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 max-width mixin #2956

Merged
merged 4 commits into from
Jul 18, 2018
Merged

Conversation

EstebanG23
Copy link
Contributor

No description provided.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

I'd like to see how exactly this will be used in #2859, since this PR mostly exists to support that. The usages currently in that PR either set this to auto (wouldn't that be equivalent to never setting max-width?), or seem to assume that an icon exists when that isn't guaranteed.

@@ -79,6 +79,7 @@ Mixin | Description
`mdc-floating-label-shake-keyframes($modifier, $positionY, $positionX, $scale)` | Generates a CSS `@keyframes` at-rule for an invalid label shake. Used in conjunction with the `mdc-floating-label-shake-animation` mixin.
`mdc-floating-label-shake-animation($modifier)` | Applies shake keyframe animation to label.
`mdc-floating-label-float-position($positionY, $positionX, $scale)` | Sets position of label when floating.
`mdc-floating-label-max-width($max-width))` | Sets the width of the label.
Copy link
Contributor

Choose a reason for hiding this comment

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

width -> max-width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -67,3 +67,9 @@
animation: mdc-floating-label-shake-float-above-#{$modifier} 250ms 1;
}
}

@mixin mdc-floating-label-max-width($max-width) {
.mdc-floating-label {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it really make sense for this selector to be baked into this mixin? Would this not ordinarily be applied to a selector that already references the floating label root element? (See also the ink/fill color mixins)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@codecov-io
Copy link

codecov-io commented Jun 18, 2018

Codecov Report

Merging #2956 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2956      +/-   ##
=========================================
- Coverage    98.3%   98.3%   -0.01%     
=========================================
  Files         101     101              
  Lines        4375    4368       -7     
  Branches      564     564              
=========================================
- Hits         4301    4294       -7     
  Misses         74      74
Impacted Files Coverage Δ
packages/mdc-icon-button/foundation.js 96.29% <0%> (-0.26%) ⬇️
packages/mdc-icon-button/index.js 100% <0%> (ø) ⬆️
packages/mdc-chips/chip-set/foundation.js 100% <0%> (ø) ⬆️

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 e823b30...ba4edfc. Read the comment docs.

@@ -79,6 +79,7 @@ Mixin | Description
`mdc-floating-label-shake-keyframes($modifier, $positionY, $positionX, $scale)` | Generates a CSS `@keyframes` at-rule for an invalid label shake. Used in conjunction with the `mdc-floating-label-shake-animation` mixin.
`mdc-floating-label-shake-animation($modifier)` | Applies shake keyframe animation to label.
`mdc-floating-label-float-position($positionY, $positionX, $scale)` | Sets position of label when floating.
`mdc-floating-label-max-width($max-width))` | Sets the max width of the label.
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't realize this was still waiting on my approval ._.

Make sure this is labeled as feat(floating-label) when you merge it.

@kfranqueiro kfranqueiro changed the title refactor(floating-label): Add max-width mixin feat(floating-label): Add max-width mixin Jul 17, 2018
@EstebanG23 EstebanG23 merged commit 66f8bf7 into master Jul 18, 2018
@jamesmfriedman jamesmfriedman mentioned this pull request Jul 30, 2018
48 tasks
@kfranqueiro kfranqueiro deleted the refactor/floating-label branch August 1, 2018 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants