-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
chore(floating-label): separate label module from text-field #2237
Conversation
…ate with mdc-text-field
|
||
@mixin mdc-text-field-label-float_($positionY, $positionX: 0%, $scale: .75) { | ||
@mixin mdc-floating-label-position_($positionY, $positionX: 0%, $scale: .75) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should be mdc-floating-label-float-position
? Is that clearer?
…-components/material-components-web into chore/text-field/decouple-label
|
||
// Private | ||
|
||
@mixin mdc-floating-label-invalid-shake-keyframes_($modifier, $positionY, $positionX: 0%, $scale: .75) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might make more sense to make this public, and move the @includes to the text-field scss file, along with the variables. Reason being select might not have the same variants (box, outlined, outlined-dense, etc).
Want to see what reviewer thinks before doing this.
Codecov Report
@@ Coverage Diff @@
## master #2237 +/- ##
==========================================
- Coverage 99.08% 98.84% -0.24%
==========================================
Files 100 100
Lines 4046 4075 +29
Branches 524 524
==========================================
+ Hits 4009 4028 +19
- Misses 37 47 +10
Continue to review full report at Codecov.
|
} | ||
|
||
// Used for textarea in case of scrolling | ||
@mixin mdc-floating-label-background-color_($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to be public
} | ||
} | ||
|
||
@mixin mdc-floating-label-shake_($modifier) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used heavily in text-field, so I think this should actually be made public, along with reasons made in https://github.com/material-components/material-components-web/pull/2237/files#r167720790
…-components/material-components-web into chore/text-field/decouple-label
``` | ||
|
||
> _NOTE_: Only place a `mdc-floating-label` inside of a text field _if you plan on using | ||
> Javascript_. Otherwise, the label must go outside of the text-field, as shown below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Javascript -> JavaScript
title: "Floating Label" | ||
layout: detail | ||
section: components | ||
excerpt: "The label is a text caption or description for the text field or select." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "An animated text caption for a text field or select."
#### Single Line, CSS Only | ||
|
||
```html | ||
<label for="text-field-no-js">TextField with no JS: </label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove trailing space just before </label>
|
||
Mixin | Description | ||
--- | --- | ||
`mdc-floating-label-color($color)` | Customizes the color of the label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`mdc-floating-label-ink-color($color)` | Customizes the ink color of the label.
- "Ink color" === CSS
color
property (usually) - "Fill color" === CSS
background-color
property (usually)
--- | --- | ||
`mdc-floating-label-color($color)` | Customizes the color of the label. | ||
`mdc-floating-label-fill-color($color)` | Customizes the fill color of the label. | ||
`mdc-floating-label-invalid-shake-keyframes($modifier, $positionY, $positionX, $scale)` | Creates a shake keyframe animation for invalid label shake. To be used with sass mixin `mdc-floating-label-shake($modifier)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generates a CSS `@keyframes` at-rule for an invalid label shake. Used in conjunction with the `mdc-floating-label-shake-animation` mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that sound much more concise
packages/mdc-textfield/adapter.js
Outdated
hasLabel() {} | ||
|
||
/** | ||
* Get width of label in pixels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add period
packages/mdc-textfield/adapter.js
Outdated
@@ -146,6 +146,33 @@ class MDCTextFieldAdapter { | |||
* @param {number} normalizedX | |||
*/ | |||
setLineRippleTransformOrigin(normalizedX) {} | |||
|
|||
/** | |||
* Shakes the floating label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there's no label? Ditto for other methods below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm good point...we have a guard in the component, but not in the foundation. I think what I'll do is move the guard into the foundation instead.
packages/mdc-textfield/adapter.js
Outdated
|
||
/** | ||
* Get width of label in pixels | ||
* @return {number} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this return if hasLabel()
is false
?
packages/mdc-textfield/foundation.js
Outdated
@@ -109,8 +111,8 @@ class MDCTextFieldFoundation extends MDCFoundation { | |||
init() { | |||
this.adapter_.addClass(MDCTextFieldFoundation.cssClasses.UPGRADED); | |||
// Ensure label does not collide with any pre-filled value. | |||
if (this.label_ && this.getValue()) { | |||
this.label_.styleFloat( | |||
if (this.getValue()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the this.hasLabel()
check?
Ditto below
input(); | ||
td.verify(label.styleShake(/* isValid */ true, /* isFocused */ true)); | ||
td.verify(label.styleFloat(/* value */ '', /* isFocused */ true, /* isBadInput */ false)); | ||
td.verify(mockAdapter.shakeLabel(/* isValid */ true, /* isFocused */ true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check with the rest of the team on this, but I would much prefer to use a destructured object instead of individual boolean params.
E.g.:
shakeLabel({isValid: true, isFocused: true})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the group has spoken and prefers inline comments, what say you?
I think with only 2 properties it is ok, but if there were more it would make more sense.
@@ -26,7 +27,7 @@ | |||
bottom: 8px; | |||
left: 0; | |||
transform-origin: left top; | |||
transition: mdc-text-field-transition(transform), mdc-text-field-transition(color); | |||
transition: mdc-floating-label-transition(transform), mdc-floating-label-transition(color); | |||
cursor: text; | |||
|
|||
// stylelint-disable plugin/selector-bem-pattern |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the block below this comment (@include mdc-rtl(".mdc-text-field")
) get moved to text-field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping it, but removing mdc-text-field. Thanks for catching
suite('MDCFloatingLabelFoundation'); | ||
|
||
test('exports cssClasses', () => { | ||
assert.isOk('cssClasses' in MDCFloatingLabelFoundation); | ||
assert.deepEqual(MDCFloatingLabelFoundation.cssClasses, cssClasses); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does isOk()
need to be changed to deepEqual()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ken said isOk just checks for truthy-ness, while isTrue is more strict and checks for being true (== vs ===). I changed to deepEqual since this covers 2 things:
- Existence, which the isOk statement is checking
- Equality
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. It looks like our other packages use a mixture of isOk
, isTrue
, and deepEqual
, which seems inconsistent and a bit odd. But OK 🙂
IMO, a deep equality check is unnecessary in this case, because it's literally comparing a value... to itself:
const {cssClasses} = MDCFloatingLabelFoundation;
...
assert.deepEqual(MDCFloatingLabelFoundation.cssClasses, cssClasses);
The object destructuring syntax above hides the fact that it's actually doing this under the hood:
assert.deepEqual(
MDCFloatingLabelFoundation.cssClasses,
MDCFloatingLabelFoundation.cssClasses);
I think the isOk
check is probably sufficient for what the test is (presumably) trying to accomplish: making sure that we remember to export cssClasses
from the foundation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kfranqueiro was saying he wanted to get rid of all isOk's in the test. However I do realize that last line you wrote is what it boils down to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose equal vs. deepEqual is a wash in this case, but either one protects us from accidentally e.g. copypasting cssClasses to strings and forgetting to actually change what it returns.
And yeah, I was saying to Matt yesterday that at some point when someone has too much time on their hands I'd like to abolish isOk/isNotOk entirely.
@@ -30,13 +30,11 @@ | |||
transition: mdc-floating-label-transition(transform), mdc-floating-label-transition(color); | |||
cursor: text; | |||
|
|||
// stylelint-disable plugin/selector-bem-pattern | |||
@include mdc-rtl(".mdc-text-field") { | |||
@include mdc-rtl() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We normally omit empty parens when defining and including Sass mixins.
Our code base is somewhat inconsistent about that, of course, since we don't have a stylelint rule for it, but just FYI 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah gotcha - thanks :)
`float(shouldFloat: boolean)` | Floats or docks the label, depending on the value of `shouldFloat`. | ||
`getWidth() => number` | Returns the width of the label element. | ||
|
||
\* **N.B.:** Multiple consecutive calls to `shake(true)` will only shake the label once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't there before and it seems like people figured out how it worked. I'll remove.
|
||
@import "./variables"; | ||
|
||
@mixin mdc-floating-label-color($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this to mdc-floating-label-ink-color
for consistency (don't forget to rename in the README too)
|
||
@mixin mdc-floating-label-float-position($positionY, $positionX: 0%, $scale: .75) { | ||
@if $positionX == 0 { | ||
.mdc-floating-label--float-above { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this selector get moved above the @if
? (And then remove the duplicate selector inside the @else
)
import MDCFloatingLabelAdapter from './adapter'; | ||
import {cssClasses} from './constants'; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove blank line
packages/mdc-floating-label/index.js
Outdated
*/ | ||
|
||
import MDCComponent from '@material/base/component'; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove blank line
@@ -101,6 +101,12 @@ | |||
} | |||
} | |||
|
|||
@mixin mdc-text-field-label-color($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add -ink
to mixin name (e.g., mdc-text-field-label-ink-color
).
Also, why was this mixin removed from the README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a lot of other mixins were not in the text-field readme. Adding them.
packages/mdc-textfield/_mixins.scss
Outdated
@include mdc-floating-label-color($color); | ||
} | ||
|
||
.mdc-text-field__input::placeholder { // placeholder used in place of label on css only version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace this line with:
// CSS-only version
.mdc-text-field__input::placeholder {
|
||
// Used for textarea in case of scrolling | ||
@mixin mdc-floating-label-fill-color($color) { | ||
.mdc-floating-label { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this selector here (we'll need to add it back to wherever the mixin is used)
$mdc-text-field-outlined-with-leading-icon-label-position-x: 32px; | ||
$mdc-text-field-outlined-dense-with-leading-icon-label-position-x: 21px; | ||
$mdc-text-field-textarea-label-position-y: 50%; | ||
// Note that the scale factor is an eyeball'd approximation of what's shown in the mocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: "eyeballed"
demos/text-field.scss
Outdated
@@ -28,7 +28,7 @@ | |||
@include mdc-text-field-hover-bottom-line-color($hover-border); | |||
@include mdc-text-field-line-ripple-color($focused-border); | |||
@include mdc-text-field-ink-color(black); | |||
@include mdc-text-field-label-color(rgba(blue, .5)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad - I didn't realize this mixin already existed. Let's undo the rename since it would be a breaking change unrelated to this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to add some parameter prefixes and fix a few nits, otherwise LGTM! Nice job Matt!
Looks like Travis is being flaky - I tested locally and everything passed.
|
||
Method Signature | Description | ||
--- | --- | ||
`shake(shouldShake: boolean)` | Shakes or stops shaking the label, depending on the value of `shouldShake`.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Remove trailing *
} | ||
|
||
// Used for textarea in case of scrolling | ||
@mixin mdc-floating-label-fill-color($color) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Move this mixin just after mdc-floating-label-ink-color
"material components", | ||
"material design", | ||
"floatinglabel", | ||
"floating-label" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Replace -
with a space
packages/mdc-textfield/_mixins.scss
Outdated
@@ -73,7 +73,9 @@ | |||
@include mdc-text-field-textarea-fill-color_($color); | |||
// Automatically add label background color the same color as well to ensure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add a blank line before this comment
opacity: 1; | ||
} | ||
|
||
@include mdc-floating-label-shake-keyframes(box, $mdc-text-field-box-label-position-y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first parameter of all of these keyframes mixins ($modifier
) should probably be
prefixed with text-field-
to avoid future collisions with mdc-select.
E.g., text-field-box
.
Other references to the keyframe names will also need to be updated.
commit f17e0d3 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Thu Mar 8 13:20:48 2018 -0500 fix(text-field): Clicking label should focus textfield (#2353) commit 77b15f4 Author: Bonnie Zhou <bzbee003@gmail.com> Date: Wed Mar 7 16:29:57 2018 -0800 refactor(button): Remove compact variant (#2361) BREAKING CHANGE: The compact variant of MDC Button is removed. commit 35a5cfc Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Tue Mar 6 19:54:23 2018 -0500 fix(toolbar): Fix colors for svg icons. Update custom-toolbar demo (#2331) SVG icons in the toolbar will now use the same color as the font icons. commit dc52201 Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Tue Mar 6 16:00:12 2018 -0800 fix(theme): Move @alternate annotations for Closure Stylesheets (#2355) `@alternate` annotations need to come before the _second_ property. Otherwise, Closure Compiler strips the first property (it does not output it at all). commit 3c04419 Author: aprigogin <17075403+aprigogin@users.noreply.github.com> Date: Tue Mar 6 14:29:12 2018 -0800 fix(button): icon in rtl should have margin right flipped. (#2346) commit b9000a4 Author: Cory Reed <swashcap@gmail.com> Date: Tue Mar 6 07:37:40 2018 -0800 fix(demos): Correct RTL/LTR toggling in demos in Safari (#2348) commit ab85736 Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Mon Mar 5 19:00:11 2018 -0500 fix: Use `var` instead of `const` in menu demo (#2345) Safari 9.x and IE 10 do not support `const` commit dc3d69f Author: aprigogin <17075403+aprigogin@users.noreply.github.com> Date: Mon Mar 5 15:22:31 2018 -0800 fix(rtl): Adding noflip annotations to fix downstream rtl issues (#2344) * fix(rtl): Adding noflip annotations to fix downstream rtl issues * fix(rtl): remove changes to button, will be in separate PR * fix(rtl): remove changes to button, will be in separate PR - second attempt. * fix(rtl): removed extra whitespace commit eb4138e Author: Patrick RoDee <prodee@google.com> Date: Mon Mar 5 14:10:46 2018 -0800 docs: Update CHANGELOG.md commit f478610 Author: Patrick RoDee <prodee@google.com> Date: Mon Mar 5 14:10:30 2018 -0800 chore: Publish commit 78408bb Author: Andrew C. Dvorak <acdvorak@gmail.com> Date: Mon Mar 5 16:13:02 2018 -0500 fix: Use `var` instead of `const` in demos/ready.js (#2343) Safari 9.x and IE 10 do not support `const`, and `ready.js` is not transpiled to ES5. commit 49a9449 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Mon Mar 5 12:21:54 2018 -0500 chore(select): Fix JS example in Readme (#2332) commit bc17291 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Fri Mar 2 13:21:07 2018 -0500 feat(top app bar): Add short top app bar always collapsed feature (#2327) commit 0ba7d10 Author: Matty Goo <mattgoo@google.com> Date: Fri Mar 2 11:33:19 2018 -0500 fix(text-field): disable validation check in setRequired (#2201) BREAKING CHANGE: removed setRequired and isRequired from foundation. commit c14d244 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Fri Mar 2 10:37:18 2018 -0500 chore(select): Fix ID values in demo (#2330) Remove unused ID and changed duplicated ID values. Also fixed the parentheses in RTL. commit ecf4060 Author: Bonnie Zhou <bzbee003@gmail.com> Date: Thu Mar 1 16:38:41 2018 -0500 feat(chips): Change chip color when selected (#2329) BREAKING CHANGE: The `mdc-chip--activated` class, `mdc-chip-activated-ink-color` Sass mixin, and the `toggleActive` methods on `MDCChip`/`MDCChipSet` have been renamed to `mdc-chip--selected`, `mdc-chip-selected-ink-color`, and `toggleSelected`, respectively. commit 4b24b51 Author: Matty Goo <mattgoo@google.com> Date: Wed Feb 28 15:20:19 2018 -0500 chore(floating-label): separate label module from text-field (#2237) BREAKING CHANGE: must use `.mdc-floating-label` selector instead of `.mdc-text-field__label` commit fd8d8d9 Author: Will Ernest <34519388+williamernest@users.noreply.github.com> Date: Wed Feb 28 14:21:55 2018 -0500 feat(top-app-bar): Implement short top app bar (#2290) Adds the short top app bar variant to the top app bar. commit a9f9bf2 Author: Kenneth G. Franqueiro <kfranqueiro@users.noreply.github.com> Date: Wed Feb 28 13:06:41 2018 -0500 docs(select): Fix front matter for label sub-component (#2323)
fixes #2126
BREAKING CHANGE: must use
.mdc-floating-label
selector instead of.mdc-text-field__label