fix(input): make input spacing match spec #4788

mmalerba · 2017-05-25T00:13:19Z

redo the input CSS to make the spacing between various elements match the material spec (https://material.io/guidelines/components/text-fields.html).

The spec shows what things look like at 16px font size, we currently use 14px which I could change, but for now I've left it so everything should look like a scaled down version of the mock.

Also adds support for icon buttons inside prefix and suffix

crisbeto

Code seems fine, but a few of the examples in the demo are broken on IE.

crisbeto · 2017-05-25T15:38:53Z

src/lib/input/input-container.scss

 .mat-input-container {
  display: inline-block;
  position: relative;
  font-family: $mat-font-family;
-  line-height: normal;
+
+  // TODO(mmalerba): should this come from typography settings? If so a lot of this CSS needs to go


There will be a typography mixin in #4375.

Ah cool, so the way I set up the styles for this, I actually need the ratio of line-height/font-size. I could try to derive this by just dividing the variables in your mat-typography-level but that expects that they have the same units. Would it be better to have the mixin take the unitless ratio?

I'm not sure whether we'd be able to exactly match the spec if we went unitless. That being said, not all values have to go through the typogaphy API necessarily. You can take the font size and have a custom line height.

Why wouldn't we be able to match the spec? For example we would just change:

$subheading: mat-typography-level(16px, 28px, 400),

to

$subheading: mat-typography-level(16px, 28 / 16, 400),

Hmm you're right, I'll change it after the initial PR gets in. It also doesn't handle margins at the moment so I can do them together. If you hardcode it and leave a comment, I can change it when I do a pass on the components.

crisbeto · 2017-05-25T15:43:31Z

src/lib/input/input-container.scss

+  .mat-datepicker-toggle {
+    width: 1em;
+    height: 1em;
+    font-size: $mat-input-prefix-suffix-icon-font-size;


This one could use a TODO to switch it to the typography API.

Actually this one I want to depend on the input's font-size so that the ratio from the spec is preserved. Therefore doesn't need to come from the typography API

jelbourn · 2017-05-25T21:03:14Z

src/lib/input/input-container.html

    <div class="mat-input-prefix" *ngIf="_prefixChildren.length">
-      <!-- TODO(andrewseguin): remove [md-prefix] -->
-      <ng-content select="[mdPrefix], [matPrefix], [md-prefix]"></ng-content>
+      <ng-content select="[mdPrefix], [matPrefix]"></ng-content>


There are three use sites for md-prefix and one for md-suffix inside google

I'll fix them

jelbourn · 2017-05-25T21:05:12Z

src/lib/input/input-container.scss

+// The height of the underline.
+$mat-input-underline-height: 1px !default;
+// The amount to scale the font for the floating label and subscript.
+$mat-input-helper-font-scale: 0.75 !default;


it's the terminology used in the spec, changed it to subscript to match our terminology

jelbourn · 2017-05-25T21:07:19Z

src/lib/input/input-container.scss

+$mat-input-wrapper-padding-bottom:
+    ($mat-input-subscript-margin-top + $mat-input-line-height) * $mat-input-helper-font-scale;
+// The amount of y-offset to apply to the placeholder when it is floating.
+$mat-input-floating-placeholder-offset: -$mat-input-infix-padding - 1em;


optional: maybe it would be useful to have a diagram for how all this comes together?

jelbourn · 2017-05-25T21:10:27Z

src/lib/input/input-container.scss

-  transform: translate3d(0, -1.35em, 0) scale($mat-input-floating-placeholder-scale-factor);
-  width: 100% / $mat-input-floating-placeholder-scale-factor;
+  transform: translateY(-$mat-input-infix-margin-top - $mat-input-infix-padding)
+             perspective(1px) // fixes text blurriness.


Huh, I've never seen this before in my life. Can you elaborate in a comment on what the problem is and how this fixes it? Any resources to link that led to this approach?

just trial and error of various approaches people suggested, linked to the site where I saw this suggestion

jelbourn · 2017-05-26T01:12:27Z

src/lib/input/input-container.scss

-             scale($mat-input-helper-font-scale);
-  width: 100% / $mat-input-helper-font-scale;
+             // fixes text blurriness, see
+             // https://css-tricks.com/forums/topic/transforms-cause-font-smoothing-weirdness-in-webkit/


Would be better to link to the relevant article instead of the question
http://www.useragentman.com/blog/2014/05/04/fixing-typography-inside-of-2-d-css-transforms/

crisbeto

Did a second pass and it LGTM. A couple of notes:

There's some weird shifting of the placeholder text that is going on in IE when you focus/blur. It's probably related to transform weirdness and we most likely can't do much about it.
I think there might a regression of the fix from fix(input): correct invalid colors #4771, but it'll probably be solved after a rebase.

mmalerba · 2017-05-31T18:37:04Z

Addressed comments

@crisbeto managed to tweak the transforms to get it to look decent on Chrome, IE, and FF

mmalerba · 2017-05-31T22:53:53Z

rebased and moved a bunch of the styles into the typography mixin, PTAL

mmalerba · 2017-05-31T23:03:12Z

src/lib/core/typography/_typography.scss

-  $button:      mat-typography-level(14px, 14px, 500)
+  $button:      mat-typography-level(14px, 14px, 500),
+  // Line-height must be unit-less fraction of the font-size.
+  $input:       mat-typography-level(16px, 1.125, 400)


@jelbourn @crisbeto spec says 16px for inputs, but by doing this I ruin its ability to inherit from its parent (e.g. so that an input inside an <h1> matches the font size, should I keep this as 16px or change to inherit?

I think the inherit is better; users can always set it to 16px if they want

jelbourn

LGTM

jelbourn · 2017-05-31T23:06:20Z

src/lib/input/_input-theme.scss

+               perspective(100px) translateZ(0.001px);
+    // The tricks above used to smooth out the animation on chrome and firefox actually make things
+    // worse on IE, so we don't include them in the IE version.
+    -ms-transform: translateY(-$mat-input-infix-margin-top - $mat-input-infix-padding)


This doesn't cause a stylelint error?

nope, seems fine

still WIP placeholder working, still WIP fixed main styles, now just themes... themes working again icon button support

angular-automatic-lock-bot · 2019-09-08T16:02:08Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

mmalerba added the pr: needs review label May 25, 2017

mmalerba assigned jelbourn, kara, crisbeto and EladBezalel May 25, 2017

mmalerba requested review from jelbourn, kara, crisbeto and EladBezalel May 25, 2017 00:13

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 25, 2017

crisbeto reviewed May 25, 2017

View reviewed changes

jelbourn reviewed May 25, 2017

View reviewed changes

jelbourn reviewed May 26, 2017

View reviewed changes

crisbeto approved these changes May 26, 2017

View reviewed changes

mmalerba force-pushed the input-spec branch 2 times, most recently from 7d41437 to 368fab8 Compare May 31, 2017 18:36

mmalerba force-pushed the input-spec branch from 368fab8 to ac89bc9 Compare May 31, 2017 22:52

mmalerba commented May 31, 2017

View reviewed changes

jelbourn approved these changes May 31, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels May 31, 2017

mmalerba added the action: merge The PR is ready for merge by the caretaker label Jun 1, 2017

mmalerba force-pushed the input-spec branch from f5f79dd to b80d23e Compare June 1, 2017 17:11

mmalerba added 5 commits June 2, 2017 13:58

make input match mocks better, WIP

bd74d52

still WIP placeholder working, still WIP fixed main styles, now just themes... themes working again icon button support

add select to baseline demo

e94a7d0

fix IE bug

b29b97b

address comments

abb8e60

address comments

c877cb9

mmalerba added 4 commits June 2, 2017 13:58

remove forced font-size

a088b2c

integrate typography system

13647fc

switch font-size to inherit

4f4f94b

fix non-floating placeholders

886025d

mmalerba force-pushed the input-spec branch from b80d23e to 886025d Compare June 2, 2017 21:01

mmalerba merged commit 16b65e8 into angular:master Jun 2, 2017

crisbeto mentioned this pull request Jun 3, 2017

Input and select misaligned #4949

Closed

jelbourn mentioned this pull request Jun 6, 2017

MdInput dimensions are not equal to those in spec #2887

Closed

mmalerba deleted the input-spec branch April 3, 2018 15:17

angular-automatic-lock-bot bot unassigned EladBezalel Sep 8, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input): make input spacing match spec #4788

fix(input): make input spacing match spec #4788

mmalerba commented May 25, 2017

crisbeto left a comment

crisbeto May 25, 2017

mmalerba May 25, 2017

crisbeto May 25, 2017

mmalerba May 25, 2017

crisbeto May 25, 2017 •

edited

Loading

crisbeto May 25, 2017

mmalerba May 25, 2017

jelbourn May 25, 2017

mmalerba May 25, 2017

jelbourn May 25, 2017

mmalerba May 25, 2017

jelbourn May 25, 2017

jelbourn May 25, 2017

mmalerba May 25, 2017

jelbourn May 26, 2017

crisbeto left a comment •

edited

Loading

mmalerba commented May 31, 2017

mmalerba commented May 31, 2017

mmalerba May 31, 2017

jelbourn May 31, 2017

jelbourn left a comment

jelbourn May 31, 2017

mmalerba May 31, 2017

angular-automatic-lock-bot bot commented Sep 8, 2019

fix(input): make input spacing match spec #4788

fix(input): make input spacing match spec #4788

Conversation

mmalerba commented May 25, 2017

crisbeto 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

crisbeto May 25, 2017 • 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

crisbeto left a comment • edited Loading

Choose a reason for hiding this comment

mmalerba commented May 31, 2017

mmalerba commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 8, 2019

crisbeto May 25, 2017 •

edited

Loading

crisbeto left a comment •

edited

Loading