[Amsterdam] Updating base text sizing #2936

daveyholler · 2020-02-26T17:25:23Z

Summary

Changing the text sizes to be calculated off of a smaller 14px base font size.
Headings (h1- h6) and EuiTitle now use $euiFontWeightBold

Consumers

The titles and headings in EUI Amsterdam are now bold. When migrating to the Amsterdam theme you'll want to pay close attention to areas like dashboards or other high-density screens to ensure that the bold headings haven't caused your layouts to break.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in IE11 and Firefox
- [ ] Props have proper autodocs
~~- [ ] Added documentation examples~~
- [ ] Added or updated jest tests
Checked for breaking changes and labeled appropriately
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

kibanamachine · 2020-02-26T17:54:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

kibanamachine · 2020-03-04T19:46:44Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

kibanamachine · 2020-03-05T00:28:48Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

snide · 2020-03-05T14:56:18Z

Do you need review on this one @daveyholler / @cchaos ?

cchaos · 2020-03-05T15:37:34Z

Sorry, no not quite yet, I'll get bette at adding the [WIP] labels and titles.

kibanamachine · 2020-03-11T23:09:31Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

cchaos

I've commented on the code to describe what's going on. Figured its easier to point to the bits. But here are some accompanying screenshots.

Text scales remain the same

Title sizes are similar but have changed slightly, mostly weight but font-size and line-height too

Components like buttons get the smaller font size while EuiText remains the same (16px for medium)

Components that used font size variables directly get basically no change unless it was the default $euiTextSize. So things like forms don't get smaller (they're already small)

cchaos · 2020-03-11T23:01:56Z

src/global_styling/mixins/_typography.scss

+  @if (map-has-key($euiTitles, $size)) {
+    @each $property, $value in map-get($euiTitles, $size) {
+      @if ($property == 'font-size') {
+        @include fontSize($value);
+      } @else {
+        #{$property}: $value;
+      }
+    }


The euiTitle() mixin now loops through the new $euiTitle map, finding the matching key first, then looping through that object and parsing the keys ('line-height') as properties and their values.

cchaos · 2020-03-11T23:02:58Z

src/global_styling/mixins/_typography.scss

+      @if ($property == 'font-size') {
+        @include fontSize($value);


This is where we need to us the fontSize() mixin to provide both px and rem values of the 'font-size' provided which should be a px value.

cchaos · 2020-03-11T23:05:35Z

src/global_styling/mixins/_typography.scss

+  @each $property, $value in map-get($euiTitles, 'm') {
+    @if ($property == 'font-size') {
+      @include fontSize($value);
+    } @else {
+      #{$property}: $value;
+    }
+  }


To avoid duplicating the same styles from the titles map, but wanting the xl & xxl large fonts to look the same as titles, we do the same procedure as in the euiTitles() mixin.

You'll see the mismatch in mixin name euiFontSizeXL to the key lookup 'm', because we didn't use a proper titling scale that matched our sass vars and now we're stuck with it 🤣

cchaos · 2020-03-11T23:06:06Z

src/global_styling/variables/_typography.scss

+@function lineHeightFromBaseline($multiplier: 3) {
+  @return convertToRem(($euiFontSize/2)*$multiplier);
+}


Needed to duplicate this mixin as a function so it could be used in the map

Maybe remove the mixin and replace with the function usage in other parts of the code?

Or at the least, use the function in the in the mixin inception style.

cchaos · 2020-03-11T23:07:50Z

src/global_styling/variables/_typography.scss

+$euiTitles: (
+  'xxxs': (
+    'font-size': $euiFontSizeXS,
+    'line-height': lineHeightFromBaseline(3),
+    'font-weight': $euiFontWeightBold,
+  ),


This is a new map that has keys that match our EuiTitle scales. You can add any property as a quoted key and it will get applied to that title size (in both EuiTitle and EuiText(size medium))

I understand this and know that it's replacing old code that did something similar, but you should likely comment on WHY this is set up this way. Letter-spacing is super uncommon in most CSS, and more so as a scale. Just some comments in here to describe why it's important would be nice for someone coming to this code for the first time.

cchaos · 2020-03-11T23:08:27Z

src/themes/eui-amsterdam/global_styling/variables/_typography.scss

@@ -0,0 +1,51 @@
+// Finally start using the non-beta version of Inter
+$euiFontFamily: 'Inter', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';


Yay for finalized Inter not Inter UI !

cchaos · 2020-03-11T23:18:34Z

src/themes/eui-amsterdam/global_styling/variables/_typography.scss

+// Font sizes -- scale is loosely based on Major Third (1.250) with a base of 14px
+// Base list is an altered scale based on 16px to match the resulted values below
+$euiTextScale:    2.125, 1.6875, 1.375, 1.125, 1, .875, .75;
+// This is a new list because changing the base list messes with EuiText
+// So it's only being used to calculate the font size variables
+$amsTextScale:    2.429, 1.929, 1.571, 1.285, 1.143, 1, .857;
+
+$euiFontSize:     $euiSize - 2; // 14px


This is where it starts to get crazy...

THE GOAL: 14px base font size

Plain ole text inherits font-size: 14px from html,body

Anything using base $euiFontSize is 14px, buttons, tabs, etc...

EuiTitle sizing are the largest they'll ever be in scale

EuiText's headings in size medium match EuiTitle sizes

However:
EuiText size medium is still a base of 16px so that we can continue to have large-form (wall of text, blog style, etc) text be larger for legibility. Which means all the text scales for EuiText pretty much remain the same. Small is 14, Xsmall is 12 just as before.

Also
EuiTitle sizes only change ever so slightly to still be a major third scale but with the base being at position 6 instead of 5 as previously. But we want the EuiText.medium heading sizes (h1-16) to match our EuiTitle sizes, so we adjust the original $euiTextScale's numbers to calculate to the equivalent of the new title sizes below. This means as it's passed to EuiText, it properly calculates off of 16px... trust me it works 😉

Note to self for review:

The only downside to something like this that I can think of is that having $euiSize and $euiFontSize different means that anything heavily relying on both (I'm thinking loose line heights) will lose some balance. Specifically, looking at some spots where line height of 1.5 (the most common value) will create odd pixel numbers against $euiSize derived values. Tabs are now 45px...etc. In isolation, this will be fine, but when components line up next to each other but have mixed value calcs... I anticipate this will cause some small breaks in places that might mean we might need to not rely on padding + font size to make the height of something. Should likely lead to more tolerant code as we spot those breaks and fix the trouble spots.

Yeah it'll be interesting to see what this effects downstream. But it's essentially the same as doing:

font-size: $euiFontSizeSmall; padding: $euiSize;

cchaos · 2020-03-11T23:19:24Z

src/themes/eui-amsterdam/global_styling/variables/_typography.scss

+$euiFontSizeXS:    ceil($euiFontSize * nth($amsTextScale, 7)); // 12px // h6
+$euiFontSizeS:    floor($euiFontSize * nth($amsTextScale, 6)); // 14px // h5 --> Now the same as the base $euiFontSize
+$euiFontSizeM:    floor($euiFontSize * nth($amsTextScale, 5)); // 16px // h4
+$euiFontSizeL:     ceil($euiFontSize * nth($amsTextScale, 3)); // 22px // h3
+$euiFontSizeXL:   floor($euiFontSize * nth($amsTextScale, 2)); // 27px // h2
+$euiFontSizeXXL:  floor($euiFontSize * nth($amsTextScale, 1)); // 34px // h1


Then the $amsTextScale is only used in this file to calculate these new font sizes, based on the new $euiFontSize;

Because $euiFontSizeS is the same as $euiFontSize, it almost makes the plain $euiFontSize moot, which is kind of a good thing since it was confusing where it'd lie within the scale.

cchaos · 2020-03-11T23:20:27Z

src/themes/eui-amsterdam/overrides/_text.scss

+.euiText--medium {
+  @include fontSize($euiFontSizeM);
+  @include euiScaleText($euiFontSizeM);
+}


This doesn't exist as a selector in the default theme. So it works great as an override selector for plain .euiText

A bonus feature of me leaving selectors that don't do anything 😈

cchaos · 2020-03-11T23:31:49Z

This PR is now ready for review.

snide

While eating a slice of pizza I made the mistake of looking at this code. These are mostly notes for when I need look closer at the output. Looking at how you wrote this I can see it has pretty much zero impact on current EUI which means it's fairly safe to merge as is. I want to do a pretty thorough scan of the display tomorrow to see what falls out from the size change. Off hand every time I think I see an AHA moment, I see the same error in current EUI.

As suspicious as I am of a change this big ... everything looks ok ⛹️‍♀️

src/themes/eui-amsterdam/global_styling/variables/_typography.scss

snide · 2020-03-12T00:58:51Z

src/themes/eui-amsterdam/global_styling/variables/_typography.scss

+// Font sizes -- scale is loosely based on Major Third (1.250) with a base of 14px
+// Base list is an altered scale based on 16px to match the resulted values below
+$euiTextScale:    2.125, 1.6875, 1.375, 1.125, 1, .875, .75;
+// This is a new list because changing the base list messes with EuiText
+// So it's only being used to calculate the font size variables
+$amsTextScale:    2.429, 1.929, 1.571, 1.285, 1.143, 1, .857;
+
+$euiFontSize:     $euiSize - 2; // 14px


Note to self for review:

The only downside to something like this that I can think of is that having $euiSize and $euiFontSize different means that anything heavily relying on both (I'm thinking loose line heights) will lose some balance. Specifically, looking at some spots where line height of 1.5 (the most common value) will create odd pixel numbers against $euiSize derived values. Tabs are now 45px...etc. In isolation, this will be fine, but when components line up next to each other but have mixed value calcs... I anticipate this will cause some small breaks in places that might mean we might need to not rely on padding + font size to make the height of something. Should likely lead to more tolerant code as we spot those breaks and fix the trouble spots.

snide · 2020-03-12T01:00:47Z

src/global_styling/variables/_typography.scss

+$euiTitles: (
+  'xxxs': (
+    'font-size': $euiFontSizeXS,
+    'line-height': lineHeightFromBaseline(3),
+    'font-weight': $euiFontWeightBold,
+  ),


I understand this and know that it's replacing old code that did something similar, but you should likely comment on WHY this is set up this way. Letter-spacing is super uncommon in most CSS, and more so as a scale. Just some comments in here to describe why it's important would be nice for someone coming to this code for the first time.

snide · 2020-03-12T01:03:14Z

src/themes/eui-amsterdam/overrides/_text.scss

+.euiText--medium {
+  @include fontSize($euiFontSizeM);
+  @include euiScaleText($euiFontSizeM);
+}


A bonus feature of me leaving selectors that don't do anything 😈

snide · 2020-03-12T01:03:57Z

src/global_styling/variables/_typography.scss

+@function lineHeightFromBaseline($multiplier: 3) {
+  @return convertToRem(($euiFontSize/2)*$multiplier);
+}


Maybe remove the mixin and replace with the function usage in other parts of the code?

src-docs/src/theme_amsterdam_light.scss

snide · 2020-03-12T01:11:31Z

src/themes/eui-amsterdam/eui_amsterdam_colors_light.scss

@@ -5,6 +5,8 @@ $euiColorPrimary:     #006DE4;
 $euiColorSecondary:   #00BFB3;
 $euiColorAccent:      #FC358E;

+$euiLinkColor: $euiColorPrimary; // Weirdly this one wasn't updating with new primary color


Note to self: look into this mystery.

snide · 2020-03-12T01:41:53Z

src/global_styling/variables/_typography.scss

+@function lineHeightFromBaseline($multiplier: 3) {
+  @return convertToRem(($euiFontSize/2)*$multiplier);
+}


Or at the least, use the function in the in the mixin inception style.

kibanamachine · 2020-03-12T04:14:27Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

kibanamachine · 2020-03-12T15:29:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

snide · 2020-03-12T20:11:40Z

@cchaos I did a more thorough scan on the display side of this and I think it looks good. I know there will be some breaks from this, specifically because of the odd pixel 1.5 line-height, but nothing in here looks dangerous in regards to current EUI themes. My questions from above still stand, but I think this is generally OK to merge and we'll just deal with any issues as we notice them.

kibanamachine · 2020-03-13T21:51:55Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_2936/

snide

Feel free to merge. We'll see what shakes out on this one over time.

daveyholler force-pushed the am/updating-text branch from 8241ae0 to 2cd96d6 Compare February 26, 2020 17:32

daveyholler requested a review from cchaos February 26, 2020 17:38

daveyholler marked this pull request as ready for review February 26, 2020 17:38

daveyholler changed the title ~~Am/updating text~~ [Amsterdam] updating text Feb 27, 2020

daveyholler added Amsterdam labels Feb 27, 2020

cchaos force-pushed the am/updating-text branch from cc52b1e to 1d7070e Compare March 4, 2020 19:28

cchaos force-pushed the am/updating-text branch from 1d7070e to 3183c31 Compare March 5, 2020 00:09

cchaos changed the title ~~[Amsterdam] updating text~~ [WIP] [Amsterdam] updating text Mar 5, 2020

cchaos added the work in progress label Mar 5, 2020

cchaos reviewed Mar 11, 2020

View reviewed changes

cchaos requested a review from snide March 11, 2020 23:31

cchaos removed the work in progress label Mar 11, 2020

cchaos changed the title ~~[WIP] [Amsterdam] updating text~~ [Amsterdam] Updating base text sizing Mar 11, 2020

snide reviewed Mar 12, 2020

View reviewed changes

daveyholler and others added 7 commits March 12, 2020 11:10

Changing font size to 14px and headings to bold

7db8d3a

Unifying font weights across EuiTitle sizes

b8d423b

Setting the scale and using mixins fo .euiText--medium

b6b5156

Finally start using the updated version of Inter

51cd9dc

A few more fixes

7118768

Using a map for title sizing

967b3ba

Fixing text scales

ddbe267

cchaos added 2 commits March 12, 2020 11:10

Using only google fonts

282532a

Remove uneccesary selector override

a41398a

cchaos force-pushed the am/updating-text branch from 6739067 to a41398a Compare March 12, 2020 15:10

cchaos added 2 commits March 13, 2020 17:15

Addressing a few review coments

a0e8a4d

cleanup

b998d02

snide approved these changes Mar 13, 2020

View reviewed changes

cchaos merged commit 2f04008 into elastic:master Mar 13, 2020

chandlerprall mentioned this pull request Mar 16, 2020

Upgrade EUI to 21.0.1 elastic/kibana#60282

Merged

		@@ -0,0 +1,51 @@
		// Finally start using the non-beta version of Inter
		$euiFontFamily: 'Inter', -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';

[Amsterdam] Updating base text sizing #2936

[Amsterdam] Updating base text sizing #2936

Conversation

daveyholler commented Feb 26, 2020 • edited Loading

Summary

Consumers

Checklist

kibanamachine commented Feb 26, 2020

kibanamachine commented Mar 4, 2020

kibanamachine commented Mar 5, 2020

snide commented Mar 5, 2020

cchaos commented Mar 5, 2020

kibanamachine commented Mar 11, 2020

cchaos 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

THE GOAL: 14px base font size

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

cchaos commented Mar 11, 2020

snide 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

kibanamachine commented Mar 12, 2020

kibanamachine commented Mar 12, 2020

snide commented Mar 12, 2020

kibanamachine commented Mar 13, 2020

snide left a comment

Choose a reason for hiding this comment

daveyholler commented Feb 26, 2020 •

edited

Loading

THE GOAL: `14px` base font size