refactor(text-field): Change text-field--box to be the new default #2859

EstebanG23 · 2018-06-01T17:16:40Z

Add changes to CSS classes to override necessary styles. Edit README to reflect the new default. Edit JS to match the changes. Change tests and constants to reflect the new changes.

Fixes #2780

Add changes to CSS classes to override necessary styles. Edit README to reflect the new default. Edit JS to match the changes. Edit constants in JS.

bonniezhou · 2018-06-05T20:25:04Z

packages/mdc-textfield/_mixins.scss

@@ -129,17 +129,21 @@
 // Baseline

 @mixin mdc-text-field-disabled_ {
-  @include mdc-text-field-bottom-line-color_($mdc-text-field-disabled-border);
+  @include mdc-text-field-bottom-line-color_($mdc-text-field-outlined-disabled-border);


We should use $mdc-text-field-disabled-border here and it should be set to 12% black (I think at least, by eyeballing the spec).

bonniezhou · 2018-06-05T20:35:03Z

packages/mdc-floating-label/_mixins.scss

+
+@mixin mdc-floating-label-width($width) {
+  .mdc-floating-label {
+    width: calc(100% - #{$width});


Update this mixin to just set width: $width

Then the calc(...) logic can go in the text field CSS, since the text field handles the icon and that's the reason for subtracting part of the width.

bonniezhou · 2018-06-05T20:35:50Z

packages/mdc-textfield/_mixins.scss

  @include mdc-text-field-ink-color_($mdc-text-field-disabled-ink-color);
  @include mdc-text-field-label-ink-color_($mdc-text-field-disabled-label-color);
  @include mdc-text-field-helper-text-color_($mdc-text-field-disabled-helper-text-color);
  @include mdc-text-field-icon-color_($mdc-text-field-disabled-icon);
  @include mdc-text-field-fullwidth-bottom-line-color_($mdc-text-field-fullwidth-bottom-line-color);
+  @include mdc-text-field-fill-color_($mdc-text-field-disabled-background);
+  @include mdc-text-field-label-color($mdc-text-field-disabled-label-color);


Line 139 can be removed, since the label color is set in line 134.

bonniezhou · 2018-06-05T20:41:02Z

packages/mdc-textfield/_mixins.scss

  pointer-events: none;

  .mdc-text-field__input {
-    border-bottom: 1px dotted;
+    border-bottom-width: 1px;
+    border-bottom-style: dotted;


We can actually remove the dotted border-bottom-style too because it's no longer in the spec for disabled text fields.

bonniezhou · 2018-06-05T20:51:22Z

packages/mdc-textfield/_mixins.scss


-  height: 56px;
+  display: inline-flex;


This can be removed because it's already set in the default text field.

bonniezhou · 2018-06-05T21:43:19Z

test/unit/mdc-textfield/mdc-text-field.test.js

+  assert.isNull(component.ripple);
+});
+
+test('#constructor sets the ripple property to `null` when given a `mdc-text-field--textarea`', () => {


nit: rename this test #constructor does not instantiate a ripple when ${cssClasses.TEXTAREA} class is present

bonniezhou · 2018-06-05T21:46:03Z

packages/mdc-floating-label/README.md

@@ -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-width($width))` | Sets width of label when floating.



Update the description to be "Sets the width of the label" since it actually sets the width of the label in both its floating and non-floating position.

bonniezhou · 2018-06-05T21:46:55Z

packages/mdc-textfield/README.md

 >
-> Other input types (such as `date`) are not currently supported.
+> Other input types (such as `number` and `date`) are not currently supported.


Nice catch!

I actually think this is also from the bad sync. I am reverting it back to how it was.

bonniezhou · 2018-06-05T21:51:10Z

packages/mdc-textfield/README.md

-`mdc-text-field-box-corner-radius($radius)` | Customizes the border radius for the text field box variant.
-`mdc-text-field-box-fill-color($color)` | Customizes the background color of the text field box.
+`mdc-text-field-corner-radius($radius)` | Customizes the border radius for the text field.
+`mdc-text-field-fill-color($color)` | Customizes the background color of the text field.



Consolidate the "Other Mixins" table into the "Mixins for Text Field" table. They don't need to be separate anymore since there's no more Box variant.

bonniezhou · 2018-06-05T21:52:05Z

packages/mdc-textfield/README.md

@@ -313,8 +305,6 @@ Method Signature | Description
 `activateFocus() => void` | Activates the focus state of the Text Field. Normally called in response to the input focus event.
 `deactivateFocus() => void` | Deactivates the focus state of the Text Field. Normally called in response to the input blur event.
 `setHelperTextContent(content: string) => void` | Sets the content of the helper text.
-`setIconAriaLabel(label: string) => void` | Sets the aria label of the icon.


Removed accidentally?

I never removed this so it must have been a bad sync on my part. I added it back!

Delete unneeded demo JS

kfranqueiro

I scanned through some of this and left a few comments.

kfranqueiro · 2018-06-06T22:09:34Z

demos/text-field.html

-          );
-        });
-      });
+      // demoReady(function() {


OOC, I'm not sure why this was commented out (I assume it's for the demo for the old default text field variant being removed), but it should be removed entirely if it's supposed to be removed

Thank you for catching that!

kfranqueiro · 2018-06-06T22:14:08Z

packages/mdc-textfield/mdc-text-field.scss

@@ -144,19 +152,6 @@
  @include mdc-text-field-dense-with-trailing-icon_;
 }

-.mdc-text-field--upgraded:not(.mdc-text-field--fullwidth):not(.mdc-text-field--box) {


I'm confused as to why this was entirely removed? AFAICT the --upgraded modifier is no longer used if we remove this.

Historically we supported text field both CSS-only and with JS, which is why this modifier existed. Support for that was dropped more recently due to all of the work going into the filled and outlined variants. I suppose pursuing support for that again would need to be a separate effort anyway; if we've collapsed these styles into the defaults, we can remove all mention of the upgraded modifier in docs and JS.

I'm particularly wondering why the height: 48px disappeared though.

I deleted all the upgraded CSS code since text-field wont be supporting CSS only.

kfranqueiro · 2018-06-06T22:16:37Z

packages/mdc-textfield/mdc-text-field.scss

@@ -186,7 +181,7 @@
    margin-bottom: 4px;
  }

-  .mdc-text-field--box + &,


I'm a little nervous about changes like this that just remove --box because whereas --box only ever applied to one variant, mdc-text-field applies to every MDC text field (thus making the --outlined selector after this completely redundant). Are we sure this is what we want, or should there be exceptions to this?

We should check this for anything where --box specifically was removed.

This was the only area where --box was removed and the .mdc-text-field part of it was left. This affects specifically the helper text that only the default and outlined variants have.

Also I deleted the outlined line because both the outline and the default variant have the .mdc-text-field class

Remove the outlined css selector because both the outlined version and the default variant (box) will have the .mdc-text-field

kfranqueiro · 2018-06-08T19:32:07Z

packages/mdc-textfield/README.md

@@ -207,12 +199,17 @@ apply these mixins with CSS selectors such as `.foo-text-field.mdc-text-field--i

 > _NOTE_: the `mdc-line-ripple-color` mixin should be applied from the not focused class `foo-text-field:not(.mdc-text-field--focused)`).

-#### Mixins for Text Field Box
+#### Mixins for Text Field


We might still want to split this up... IIUC ink/label color are applicable to any text field, while the others are specific to the filled text field (i.e. they wouldn't make sense on an outlined text field).

I separated them into Mixings specific for Filled Text Fields and All text fields

kfranqueiro · 2018-06-08T19:37:01Z

packages/mdc-textfield/_mixins.scss


  pointer-events: none;

  .mdc-text-field__input {
-    border-bottom: 1px dotted;
+    // border-bottom-width: 1px;


Remove this selector entirely if the styles are going away

Completely right, I forgot to delete the commented code. Got it!

kfranqueiro · 2018-06-08T19:39:03Z

packages/mdc-textfield/_mixins.scss

    line-height: 1.15;
-    pointer-events: none;
+    text-overflow: clip;


I don't understand why these 4 styles were added - especially pointer-events: auto, didn't we recently intentionally set that to none to avoid the label stealing clicks from the input?

I am trying to recreate the same effects that the text fields had before and so since more CSS was added in to the default .mdc-text-field I added styles to counteract the changes. However, Pointer-events actually does not counteract anything so i took that one out.

kfranqueiro · 2018-06-08T19:39:47Z

packages/mdc-textfield/_mixins.scss

@@ -176,12 +178,16 @@
 }

 @mixin mdc-text-field-dense_ {
-  // NOTE: This is an eyeball'd approximation of what's in the mocks.
-  @include mdc-floating-label-float-position(110%, 0%, $mdc-text-field-dense-label-scale);
+  @include mdc-floating-label-float-position($mdc-text-field-dense-label-position-y, 0%, $mdc-text-field-dense-label-scale);


This was changed from 110% to 70%?

110% was the value for the variant being deprecated and the new default has a value of 70%. The only two variants this affects is the outlined and the "box" however the outlined has its own value in dense.

kfranqueiro · 2018-06-08T19:50:22Z

packages/mdc-floating-label/mdc-floating-label.scss

    cursor: text;
+    overflow: hidden;
+    // Force the label into its own layer to prevent to prevent visible layer promotion adjustments


"to prevent" is repeated

Also, this sounds like something that should be a separate fix, not in this PR...

I can create a PR for that.

kfranqueiro · 2018-06-08T19:52:21Z

packages/mdc-textfield/_mixins.scss

-    @include mdc-rtl-reflexive-position(left, 16px);
-
-    position: absolute;
-    bottom: 20px;


I'm a bit confused why this was moved to a common mixin used for both filled and outlined text fields, if it was formerly specific to outlined?

It was specified in the outlined mixin but actually it was used in both the box variant and the outlined.

kfranqueiro · 2018-06-08T19:54:12Z

packages/mdc-textfield/_mixins.scss

  &:not(.mdc-text-field--textarea) {
+    @include mdc-states-base-color(transparent);


I'm seeing states overridden to transparent several places in this file. I would sooner add :not selectors to our application of states styles than override it everywhere else (which will create far more styles in the output CSS; try npm run build and see). Is there a reason you didn't do that?

i think per our conversation its better to not have them too specific for the user to be able to customize it?

kfranqueiro · 2018-06-08T19:54:52Z

packages/mdc-textfield/_mixins.scss

@@ -473,6 +413,8 @@
  // to position the label within the bounds of the inset padding.
  @include mdc-floating-label-float-position($mdc-text-field-textarea-label-position-y, 0%, $mdc-text-field-textarea-label-scale);
  @include mdc-floating-label-shake-animation(textarea);
+  @include mdc-states-base-color(transparent);


See previous comment RE excluding selectors for states styles rather than overriding

Same as before.

kfranqueiro · 2018-06-08T19:57:33Z

packages/mdc-textfield/_mixins.scss

-    height: auto;
+    align-self: auto;
+    box-sizing: content-box;
+    height: 30px;


I am confused by why this is changed

Possibly related: this screenshot diff I took of part of the page that otherwise didn't change

(Also notice the label position changed)

Doesnt seem to show a difference in my comparison with master. Will follow up with you

kfranqueiro · 2018-06-08T20:15:20Z

This is also definitely a bug:

… and split readme mixins

patrickrodee · 2018-06-18T21:39:54Z

packages/mdc-textfield/README.md


 Mixin | Description
 --- | ---
-`mdc-text-field-box-corner-radius($radius)` | Customizes the border radius for the text field box variant.
-`mdc-text-field-box-fill-color($color)` | Customizes the background color of the text field box.
+`mdc-text-field-ink-color($color)` | Customizes the text entered into the text field.


"Customizes the color of the text..."

patrickrodee · 2018-06-18T21:43:56Z

packages/mdc-textfield/_mixins.scss

  border: none;
+  overflow: visible;


Could you explain what this does in the context of the outlined textfield and why it's necessary to make the content overflow visible?

In default, overflow is set to visible. When moving the Box variant to the new default it made overflow to hidden in the mdc-text-field class. It is necessary to set it to visible because when the label floats to the border of the box if it was not visible then the top half of the text would be cut.

codecov-io · 2018-06-21T00:01:44Z

Codecov Report

Merging #2859 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2859      +/-   ##
==========================================
+ Coverage   98.37%   98.39%   +0.01%     
==========================================
  Files         123      123              
  Lines        5185     5185              
  Branches      639      639              
==========================================
+ Hits         5101     5102       +1     
+ Misses         84       83       -1

Impacted Files	Coverage Δ
packages/mdc-textfield/constants.js	`100% <ø> (ø)`	⬆️
packages/mdc-textfield/index.js	`93.78% <100%> (+0.62%)`	⬆️

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 5007604...6989b93. Read the comment docs.

googlebot · 2018-06-21T00:01:48Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

kfranqueiro · 2018-06-21T14:57:38Z

Something went wrong the last time this branch was synced. I'm adding the do-not-merge label until that's sorted out.

…orrect comments

…ts/material-components-web into text-field/box-default

williamernest · 2018-08-20T18:31:28Z

Latest screenshot test with known helper text diffs.
https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/20/17_13_58_957/report/report.html

kfranqueiro · 2018-08-22T14:29:40Z

packages/mdc-textfield/mdc-text-field.scss

    cursor: auto;
  }
  // stylelint-enable plugin/selector-bem-pattern
 }

+.mdc-floating-label {


I'm pretty sure all of these styles except for pointer-events: none; are now redundant with styles in the mdc-floating-label package.

Also, there's lint complaints on this line.

112:1 ✖ Invalid component selector ".mdc-floating-label" plugin/selector-bem-pattern 112:1 ✖ Expected selector ".mdc-floating-label" to come before selector ".mdc-text-field__input:-webkit-autofill + .mdc-floating-label" no-descending-specificity

kfranqueiro · 2018-08-22T14:39:22Z

packages/mdc-textfield/README.md

@@ -202,12 +201,23 @@ apply these mixins with CSS selectors such as `.foo-text-field.mdc-text-field--i

 > _NOTE_: the `mdc-line-ripple-color` mixin should be applied from the not focused class `foo-text-field:not(.mdc-text-field--focused)`).

-#### Mixins for Text Field Box
+#### Mixins for all Text Field


This heading doesn't seem quite accurate... at the very least, the label color mixin is only applicable to filled/outlined, not full-width / textarea (which don't use floating label).

The label color mixin applies to fullwidth and textarea.

kfranqueiro · 2018-08-22T14:53:07Z

demos/theme/index.html

@@ -800,7 +800,7 @@ <h3 class="mdc-typography--headline5 demo-component-section__heading">
          </p>
        </figure>
        <figure class="demo-text-field-wrapper">
-          <div class="mdc-text-field mdc-text-field--box demo-text-field">
+          <div class="mdc-text-field demo-text-field">


The input and helper text under this text field still has an id that says box, do we want to update that?

kfranqueiro · 2018-08-22T14:56:22Z

packages/mdc-textfield/mdc-text-field.scss

+
+  // Floating Label
+  @include mdc-text-field-floating-label_;
+  // The following mixins will be removed after select is updated to default to the box.


I don't understand this comment or the shake-animation/keyframes taking text-field-box as an identifier. The mdc-select package doesn't depend on mdc-text-field at all, so what is this doing? The only reference I see to this within Select's own styles is with text-field-outlined.

This comment is stating that this isn't being updated until the floating-label is updated after the select. Updating these requires me to change things in the floating-label package, which is not within the scope of this PR.

mdc-web-bot · 2018-08-22T17:55:56Z

🤖 Beep boop!

Screenshot test report 🚦

144 screenshots changed from master on commit a38eb43:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/22/17_54_20_711/report/report.html

Details

144 Changed:

mdc-web-bot · 2018-08-22T18:10:13Z

🤖 Beep boop!

Screenshot test report 🚦

144 screenshots changed from master on commit e4a08c8:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/22/18_08_44_605/report/report.html

Details

144 Changed:

kfranqueiro · 2018-08-23T14:15:18Z

Did this regress #3332?

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/22/18_08_44_605/report/report.html#spec/mdc-textfield/issues/3332.html:desktop_windows_chrome@latest

williamernest · 2018-08-23T14:17:46Z

No, it wasn't using the correct class for height/width in the golden.

Stale review

mdc-web-bot · 2018-08-23T17:55:00Z

🤖 Beep boop!

Screenshot test report 🚦

144 screenshots changed from master on commit 6989b93:

https://storage.googleapis.com/mdc-web-screenshot-tests/travis/2018/08/23/17_53_37_773/report/report.html

Details

144 Changed:

EstebanG23 added 3 commits May 31, 2018 14:54

refactor(text-field): Update code to make box the new default

3f19f3a

Add changes to CSS classes to override necessary styles. Edit README to reflect the new default. Edit JS to match the changes. Edit constants in JS.

refactor(text-field): Add mixins, fix tests, edit readmes

cdc9fff

refactor(text-field): Change demo page to reflect new default component

3d27fe3

EstebanG23 requested review from lynnmercier, bonniezhou and patrickrodee June 1, 2018 17:16

bonniezhou self-assigned this Jun 1, 2018

bonniezhou previously requested changes Jun 5, 2018

View reviewed changes

refactor(text-field): Address changes requested

953fba8

Delete unneeded demo JS

kfranqueiro suggested changes Jun 6, 2018

View reviewed changes

EstebanG23 added 2 commits June 6, 2018 17:38

refactor(text-field): Delete commented code

ba85626

refactor(text-field): Remove unneeded code

410f4be

Remove the outlined css selector because both the outlined version and the default variant (box) will have the .mdc-text-field

kfranqueiro suggested changes Jun 8, 2018

View reviewed changes

EstebanG23 added 7 commits June 11, 2018 17:54

refactor(text-field): Change width to Max-width, delete unneeded code…

f9e22ae

… and split readme mixins

Merge branch 'maintenance/text-field' into text-field/box-default

e60affc

Merge branch 'maintenance/text-field' into text-field/box-default

e6d47e7

refactor(floating-label): Edit comment

761317e

refactor(floating-label): Delete changes not for this PR

9363932

refactor(floating-label): Delete changes not for this PR

2e4907f

Merge branch 'maintenance/text-field' into text-field/box-default

97c59df

patrickrodee previously requested changes Jun 18, 2018

View reviewed changes

EstebanG23 added 2 commits June 20, 2018 16:58

refactor(text-field): Move floating label max-width declaration

a032dc2

Merge branch 'master' into text-field/box-default

6831617

kfranqueiro added the do-not-merge label Jun 21, 2018

refactor(text-field): Edit height of text area, correct readme, and c…

17957b6

…orrect comments

acdvorak and others added 7 commits August 14, 2018 18:54

Merge remote-tracking branch 'origin/master' into text-field/box-default

0fdec4f

Merge branch 'text-field/box-default' of github.com:material-componen…

f3bb749

…ts/material-components-web into text-field/box-default

WIP: Add missing space

c0730dc

WIP: Re-indent HTML

b7fe584

WIP: Fix linter errors

5c656c9

fix(text-field): Update outlined styles to match screenshots

0b0ad65

Merge branch 'master' into text-field/box-default

3a3a1fc

trimox mentioned this pull request Aug 16, 2018

mdc-text-field: box property not working trimox/angular-mdc-web#1227

Closed

williamernest and others added 2 commits August 17, 2018 11:34

Merge branch 'master' into text-field/box-default

8259a5d

fix(text-field): Update tests/readme to remove --box

152d6d3

fix(text-field): Remove content-box from outlined variant

852f3a5

kfranqueiro mentioned this pull request Aug 20, 2018

Input not display correctly since 0.39.0-0 #3381

Closed

williamernest removed the do-not-merge label Aug 20, 2018

kfranqueiro suggested changes Aug 22, 2018

View reviewed changes

williamernest added 2 commits August 22, 2018 09:07

fix(text-field): Update for comments. Fix theme demo

f245206

fix(text-field): Update goldens

a38eb43

Merge branch 'master' into text-field/box-default

e4a08c8

kfranqueiro approved these changes Aug 23, 2018

View reviewed changes

Merge branch 'master' into text-field/box-default

6989b93

williamernest merged commit 01b6be7 into master Aug 23, 2018

williamernest deleted the text-field/box-default branch August 23, 2018 17:58

alphabt mentioned this pull request Sep 2, 2018

Change select--box to be the new default #3495

Closed

		&:not(.mdc-text-field--textarea) {
		@include mdc-states-base-color(transparent);

refactor(text-field): Change text-field--box to be the new default #2859

refactor(text-field): Change text-field--box to be the new default #2859

Conversation

EstebanG23 commented Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfranqueiro commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jun 21, 2018 • edited Loading

Codecov Report

googlebot commented Jun 21, 2018

kfranqueiro commented Jun 21, 2018

williamernest commented Aug 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Aug 22, 2018

Screenshot test report 🚦

144 Changed:

mdc-web-bot commented Aug 22, 2018

Screenshot test report 🚦

144 Changed:

kfranqueiro commented Aug 23, 2018

williamernest commented Aug 23, 2018 • edited Loading

mdc-web-bot commented Aug 23, 2018

Screenshot test report 🚦

144 Changed:

EstebanG23 commented Jun 1, 2018 •

edited

Loading

codecov-io commented Jun 21, 2018 •

edited

Loading

williamernest commented Aug 23, 2018 •

edited

Loading