feat: integrate typography api into all components #4375

crisbeto · 2017-05-03T18:55:13Z

Includes the base typography styles for headers, body text etc.
Adds mixins for typography next to the theme and moves out all of the component typography styles into them.
Switches all the hardcoded typography values to use the ones from the spec.
Adjusts the letter spacing on some of the headers to align them to the spec.

Note: While going through the components, I haven't checked whether the styles are accurate in regards to the spec. I've left notes in the places where the font sizes weren't a part of the pre-defined ones.

jelbourn · 2017-05-03T23:24:56Z

src/demo-app/demo-app/demo-app.html

-        </button>
+        <div>
+          <button md-button (click)="toggleFullscreen()" title="Toggle fullscreen">
+            Fullscreen


Since you're incidentally touching this, can you right-align this and make it an md-icon-button with the fullscreen icon 😄 ? That's been bothering me forever.

jelbourn · 2017-05-03T23:35:47Z

src/demo-app/typography/typography-demo.html

+<h6 class="mat-h6">Lorem ipsum dolor sit amet, consectetur adipisicing.</h6>
+
+<div class="mat-body">
+  <p>Lorem ipsum dolor sit amet, <span class="mat-body-strong">consectetur</span> adipisicing elit. Iure voluptatem amet facilis rerum non dolore repellendus ab. Assumenda nisi perspiciatis odit illum voluptatem expedita laborum! Debitis nisi eius, ratione nostrum velit dignissimos molestias saepe, esse facilis blanditiis, labore optio. Accusantium, nihil illo est beatae nobis expedita animi libero, laboriosam excepturi consequatur eaque, ab repudiandae.</p>


optional: I like adding something more meaningful / whimsical for demos, up to you

jelbourn · 2017-05-03T23:43:28Z

src/lib/chips/_chips-theme.scss

@@ -1,5 +1,11 @@
 @import '../core/theming/palette';
 @import '../core/theming/theming';
+@import '../core/typography/typography-utils';
+
+// TODO(crisbeto): these values don't correspond to any of the typography breakpoints.


We might have to invent our own; the spec often paints broad strokes that don't capture the specific details for each component

jelbourn · 2017-05-03T23:46:40Z

src/lib/core/typography/_typography.scss

  .mat-h0, .mat-hero-header {
    @include mat-typography-level-to-styles($config, display-4);
+    letter-spacing: -0.05em;


Does the letter spacing come from the spec somewhere?

Not really. I arrived at it by overlaying the spec and tweaking ours until they lined up.

I vote for undoing this and just using default spacing, some things look kind of cramped when using these settings, for example the page header.

Page header before this PR:

Page header after this PR:

This one isn't using the letter spacing at all @mmalerba, the spacing only applies to h1, h2 and hero header. I've reduced the spacing only on the larger headers.

Ah I see, it seems it switched from Arial to Roboto which explains the difference, maybe we should add .mat-h4/5 to the element so it looks a little better?

You're right, it's because we have a global style for body h1 in the demo app, which ends up overriding the toolbar style. I'll push a fix.

Add a comment somewhere explaining where the values came from, then? I know a year from now I'll be looking at this and be thinking "Where did these come from?!"

I already left a link to the spec right above the pre-defined values.

@crisbeto the spec doesn't explicitly give the letter spacing, though; we should document where those specific values came from.

Ah I see. As I mentioned, I arrived at them mainly by overlaying the spec over our demo page and tweaking it until they aligned. I'll add notes about it.

jelbourn · 2017-05-03T23:51:52Z

@tinayuangao @andrewseguin @kara @mmalerba can you each do a quick pass on the components you own here?

mmalerba · 2017-05-04T23:05:53Z

LGTM for the input, slider, and sidenav changes

tinayuangao · 2017-05-04T23:53:50Z

LGTM for checkbox, button, button-toggle and radio-button

crisbeto · 2017-05-08T19:00:25Z

Addressed the feedback @jelbourn.

jelbourn · 2017-05-10T22:17:49Z

@kara @andrewseguin please take a look

No idea what's going on with the screenshot tests

kara · 2017-05-15T23:01:01Z

@crisbeto Rebase?

crisbeto · 2017-05-16T17:37:52Z

Rebased @kara.

andrewseguin · 2017-05-16T18:15:57Z

LGTM for tooltip/tabs

kara

@crisbeto I think the rebase may have caused issues. I'm seeing this:

crisbeto · 2017-05-16T18:36:09Z

Fixed @kara. I had forgotten to prefix the theme file name with an underscore and it ended up throwing a SASS error.

kara

LGTM

jelbourn · 2017-05-17T18:50:45Z

@crisbeto removing the $mat-font-family breaks a bunch of Google apps; can we add it back for know so we can separately refactor it away?

crisbeto · 2017-05-17T18:54:56Z

Done @jelbourn.

jelbourn · 2017-05-17T21:31:53Z

Looks like that fixed the errors, but I have to update some internal build rules and screenshot tests to get this submitted, so I'll do a separate sync just for this.

* Includes the base typography styles for headers, body text etc. * Adds mixins for typography next to the theme and moves out all of the component typography styles into them. * Switches all the hardcoded typography values to use the ones from the spec. * Adjusts the letter spacing on some of the headers to align them to the spec. **Note:** While going through the components, I haven't checked whether the styles are accurate in regards to the spec. I've left notes in the places where the font sizes weren't a part of the pre-defined ones.

mmalerba · 2017-05-31T16:34:27Z

@crisbeto Inspecting the demo app, I see that the mat-button-wrapper for the hamburger icon now has a computed font-size of 13.3333px rather than the 14px it previously had. Do you know what's responsible for this change? Is it expected? (It seems to be the cause of a lot of the screenshot diffs I'm seeing in Google)

angular-automatic-lock-bot · 2019-09-06T13:02:17Z

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.}

crisbeto added the pr: needs review label May 3, 2017

crisbeto assigned jelbourn May 3, 2017

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

crisbeto force-pushed the typography-2 branch from 5928875 to a34d89a Compare May 3, 2017 19:00

jelbourn reviewed May 3, 2017

View reviewed changes

kara assigned crisbeto and unassigned jelbourn May 15, 2017

kara added the pr: needs rebase label May 15, 2017

crisbeto force-pushed the typography-2 branch from 602a93b to 1509bb3 Compare May 16, 2017 17:36

crisbeto removed the pr: needs rebase label May 16, 2017

kara suggested changes May 16, 2017

View reviewed changes

kara approved these changes May 16, 2017

View reviewed changes

kara unassigned crisbeto May 16, 2017

kara added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed cla: yes PR author has agreed to Google's Contributor License Agreement pr: needs review labels May 16, 2017

jelbourn added presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels May 17, 2017

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

jelbourn removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 17, 2017

tinayuangao added the pr: needs rebase label May 24, 2017

crisbeto force-pushed the typography-2 branch from 666522c to 6709838 Compare May 24, 2017 20:03

crisbeto mentioned this pull request May 25, 2017

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

Merged

crisbeto removed the pr: needs rebase label May 25, 2017

crisbeto added 6 commits May 27, 2017 10:11

fix: address feedback

3413d17

fix: explicit toolbar header typography

6cadb4c

chore: add comments about letter spacing

625f55e

fix: theme error

2cda821

chore: re-add $mat-font-family

0066e48

crisbeto force-pushed the typography-2 branch from 6709838 to 0066e48 Compare May 27, 2017 08:13

fix: missing typography for mat-icon-button

75c8952

mmalerba merged commit e650b04 into angular:master May 31, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate typography api into all components #4375

feat: integrate typography api into all components #4375

crisbeto commented May 3, 2017

jelbourn May 3, 2017

jelbourn May 3, 2017

jelbourn May 3, 2017

jelbourn May 3, 2017

crisbeto May 4, 2017

mmalerba May 4, 2017

crisbeto May 8, 2017 •

edited

Loading

mmalerba May 8, 2017

crisbeto May 9, 2017

jelbourn May 10, 2017

crisbeto May 11, 2017

jelbourn May 11, 2017

crisbeto May 11, 2017

jelbourn commented May 3, 2017

mmalerba commented May 4, 2017

tinayuangao commented May 4, 2017

crisbeto commented May 8, 2017

jelbourn commented May 10, 2017

kara commented May 15, 2017

crisbeto commented May 16, 2017

andrewseguin commented May 16, 2017

kara left a comment

crisbeto commented May 16, 2017

kara left a comment

jelbourn commented May 17, 2017

crisbeto commented May 17, 2017

jelbourn commented May 17, 2017

mmalerba commented May 31, 2017

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

feat: integrate typography api into all components #4375

feat: integrate typography api into all components #4375

Conversation

crisbeto commented May 3, 2017

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 May 8, 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

jelbourn commented May 3, 2017

mmalerba commented May 4, 2017

tinayuangao commented May 4, 2017

crisbeto commented May 8, 2017

jelbourn commented May 10, 2017

kara commented May 15, 2017

crisbeto commented May 16, 2017

andrewseguin commented May 16, 2017

kara left a comment

Choose a reason for hiding this comment

crisbeto commented May 16, 2017

kara left a comment

Choose a reason for hiding this comment

jelbourn commented May 17, 2017

crisbeto commented May 17, 2017

jelbourn commented May 17, 2017

mmalerba commented May 31, 2017

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

crisbeto May 8, 2017 •

edited

Loading