[Emotion][Theming] Add new consumer-configurable `font.defaultUnits` token #7133

cee-chen · 2023-08-28T19:41:12Z

Summary

Currently, all Emotion component font sizes use rem sizing and this isn't something that we allow configuration for (e.g. to px or em) values. Adding a new font.defaultUnits token to our EUI theme will enable consumers to easily and quickly customize our component font size CSS output in one place.

⚠️ Note that there are a few components that do need to set a custom measurement override that does not use this new token - primarily <EuiText size="relative"> is the (current) one instance of this in EUI where em is enforced.

Screencap of example usage:

QA

Go to http://localhost:8030/#/theming/typography/values
Inspect the EuiText wrapper around the example text and confirm both its font-size and line-height CSS properties are using px and not rem

General checklist

Props have proper autodocs (using @default if default values are missing) and playground toggles
Added documentation
Added or updated jest ~~and cypress tests~~
A changelog entry exists and is marked appropriately

kibanamachine · 2023-08-28T20:08:30Z

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

…ont size unit

+ write missing unit tests for existing utilities

+ with example override behavior

cee-chen · 2023-08-28T20:18:17Z

src/themes/amsterdam/global_styling/variables/_typography.ts

@@ -34,6 +34,7 @@ export const fontBase: _EuiThemeFontBase = {

  // Careful using ligatures. Code editors like ACE will often error because of width calculations
  featureSettings: "'calt' 1, 'kern' 1, 'liga' 1",
+  fontSizeUnit: 'rem',


[naming things is hard] now that I realize line-height is using this as well as font-size, I'm wondering if this var name still makes sense. Maybe just

Suggested change

fontSizeUnit: 'rem',

sizeUnit: 'rem',

or

Suggested change

fontSizeUnit: 'rem',

unit: 'rem',

orrrr

Suggested change

fontSizeUnit: 'rem',

defaultUnits: 'rem',

??

I lean toward defaultUnits myself. When I see the object font.defaultUnits I immediately think "pixesl, ems, or rems."

I'm leaning towards defaultUnits as well! Going to make that change here 🎉

Token rename: c94844c

Far more opinionated rename of all measurement-adjacent variable names to unit instead: 7d3c34a

I'd be curious to hear what you think about the 2nd commit - for me personally, as a dev with over a decade of CSS experience, measurement means very little to me, whereas unit has a very specific connotation within CSS font sizes and I feel it more accurately describes the enum. That being said, I'm being pretty opinionated with that change, so if you're not a fan of it or are worried about downstream implications, I'm definitely open to reverting it!

I'm looking at the changes now and I think you're onto something with this new naming convention.

To my thinking

16 is a measurement

px are the units

Units mean a lot more to me too. The functions are clearer with the new names. I trust your call to make this change and support it 100%.

Great call on the distinction. It sounds like you're good with moving ahead with the full rename - if so, I'll merge the PR once you've approved. Thanks again Trevor!

kibanamachine · 2023-08-28T20:40:02Z

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

1Copenut

Just two comments on naming. I'll update my review to Approved when teammates have had a chance to weigh in.

1Copenut · 2023-08-28T22:18:22Z

src/themes/amsterdam/global_styling/variables/_typography.ts

@@ -34,6 +34,7 @@ export const fontBase: _EuiThemeFontBase = {

  // Careful using ligatures. Code editors like ACE will often error because of width calculations
  featureSettings: "'calt' 1, 'kern' 1, 'liga' 1",
+  fontSizeUnit: 'rem',


I lean toward defaultUnits myself. When I see the object font.defaultUnits I immediately think "pixesl, ems, or rems."

src/global_styling/variables/typography.ts

…riables to `unit` instead - this feels like a far more understandable nomenclature that native CSS already uses - it's technically a breaking change, but I'm not even sure who's using these types or utilities, so tbh I'm more inclined to file it under our Emotion header

kibanamachine · 2023-08-28T23:36:45Z

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

1Copenut

💯 LGTM!

kibanamachine · 2023-08-29T18:11:59Z

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

elasticmachine · 2023-08-29T18:22:22Z

💚 Build Succeeded

Buildkite Build
Commit: 52e5f90

History

💚 Build #472 succeeded 7d3c34a
💚 Build #470 succeeded 56125c7

## PR change summary `v87.2.0`⏩`v88.1.0` ⚠️ The biggest thing to QA in this PR is several **breaking changes** to `EuiDescriptionList`. ### Description list `columnWidths` prop This PR introduces a new `columnWidths` prop and removes several Kibana instances of custom CSS overrides to title/description column widths. The primary motivation behind this is not just to reduce custom CSS, but also because v88.0.0 introduced an underlying CSS change of `column` description lists to using [`display: grid` CSS](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_grid_layout). The new prop allows us to support existing description list custom widths while not requiring Kibana developers to understand or write grid CSS (except for 1 or two scenarios around `max-width`). ⚠️ **No user-facing UI around column widths should have regressed as a result of these changes. If they have, please let us know in this PR.** ### Description list gutter size changes The prop `gutterSize` has been renamed to `rowGutterSize` and the default size is now `s` instead of `m`. The change to `s` from `m` means there is an **expected** smaller gap between list items (see below screenshots): **Current `EuiDescriptionList` with default `rowGutterSize="s"`** <img width="753" alt="" src="https://github.com/elastic/kibana/assets/934879/c17aef28-ed3b-40c4-84c3-396e788b13bb"> **Prior `EuiDescriptionList` with default `gutterSize="m"`** <img width="721" alt="" src="https://github.com/elastic/kibana/assets/934879/84d5f5a2-8fa6-4f99-9dc0-73fd143aa1e1"> If Kibana teams prefer to keep the previous `m` gutter for their instances of `EuiDescriptionList`, you have a couple of options: 1. Let EUI team know in the PR and we can set usage back to what it was before 2. Set `rowGutterSize="m"` yourselves manually --- ## [`88.1.0`](https://github.com/elastic/eui/tree/v88.1.0) - Added `font.defaultUnits` theme token. EUI component font sizes default to `rem` units - this token allows consumers to configure this to `px` or `em` ([#7133](elastic/eui#7133)) - Updated `EuiDescriptionList` with new `columnWidths` prop ([#7146](elastic/eui#7146)) **Bug fixes** - Fixed `EuiDataGrid`'s keyboard shortcuts popover display ([#7146](elastic/eui#7146)) **CSS-in-JS conversions** - Renamed `useEuiFontSize()`'s `measurement` option to `unit` for clarity ([#7133](elastic/eui#7133)) ## [`88.0.0`](https://github.com/elastic/eui/tree/v88.0.0) - Updated `EuiDescriptionList` with a new `columnGutterSize` prop ([#7062](elastic/eui#7062)) **Deprecations** - Deprecated `EuiSuggest`. We recommend using `EuiSelectable` or `EuiComboBox` instead ([#7122](elastic/eui#7122)) - Deprecated `EuiControlBar`. We recommend using `EuiBottomBar` instead ([#7122](elastic/eui#7122)) - Deprecated `EuiColorStops`. We recommend copying the component to your application if necessary ([#7122](elastic/eui#7122)) - Deprecated `EuiNotificationEvent`. We recommend copying the component to your application if necessary ([#7122](elastic/eui#7122)) **Breaking changes** - Renamed `EuiDescriptionList`'s `gutterSize` prop to `rowGutterSize` ([#7062](elastic/eui#7062)) - `EuiDescriptionList`'s `rowGutterSize` prop now defaults to a size of `s` (was previously `m`) ([#7062](elastic/eui#7062)) **Accessibility** - Fixed the dark mode colors of inline `EuiDescriptionListTitle`s to meet WCAG color contrast requirements ([#7062](elastic/eui#7062)) **CSS-in-JS conversions** - Converted `EuiKeyPadMenuItem` to Emotion; Removed `$euiKeyPadMenuSize` and `$euiKeyPadMenuMarginSize` ([#7118](elastic/eui#7118)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Nikita Indik <nikita.indik@elastic.co>

cee-chen added 5 commits August 28, 2023 13:16

Add new theme token for allowing consumers to customize the default f…

22e8e98

…ont size unit

Update EUI font size functions to use new unit token as a default

79c59a6

+ write missing unit tests for existing utilities

Add section to typography token docs

6e93f44

+ with example override behavior

Fix EuiText and EuiMarkdownFormat to use new token default

648e3b2

changelog

56125c7

cee-chen force-pushed the font-unit-token branch from d3e6450 to 56125c7 Compare August 28, 2023 20:16

cee-chen marked this pull request as ready for review August 28, 2023 20:16

cee-chen requested review from a team and 1Copenut August 28, 2023 20:16

cee-chen commented Aug 28, 2023

View reviewed changes

1Copenut reviewed Aug 28, 2023

View reviewed changes

cee-chen added 2 commits August 28, 2023 15:38

Rename fontSizeUnit to defaultUnits

c94844c

cee-chen changed the title ~~[Emotion][Theming] Add new consumer-configurable fontSizeUnit token~~ [Emotion][Theming] Add new consumer-configurable font.defaultUnits token Aug 28, 2023

1Copenut approved these changes Aug 29, 2023

View reviewed changes

quick last copy tweak

52e5f90

cee-chen enabled auto-merge (squash) August 29, 2023 17:28

cee-chen mentioned this pull request Aug 29, 2023

Support specifying _FontScaleOptions globally #7124

Closed

cee-chen mentioned this pull request Aug 29, 2023

[a11y] EUI does not respect user/browser default font sizes #7143

Closed

cee-chen merged commit 86e6702 into elastic:main Aug 29, 2023

cee-chen deleted the font-unit-token branch August 29, 2023 18:22

cee-chen mentioned this pull request Aug 30, 2023

Upgrade EUI to v88.1.0 elastic/kibana#165047

Merged

cee-chen mentioned this pull request Sep 12, 2023

Update global EUI font size to use configurable theme units #7182

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion][Theming] Add new consumer-configurable `font.defaultUnits` token #7133

[Emotion][Theming] Add new consumer-configurable `font.defaultUnits` token #7133

cee-chen commented Aug 28, 2023 •

edited

Loading

kibanamachine commented Aug 28, 2023

cee-chen Aug 28, 2023 •

edited

Loading

1Copenut Aug 28, 2023

cee-chen Aug 28, 2023

cee-chen Aug 28, 2023 •

edited

Loading

1Copenut Aug 29, 2023 •

edited

Loading

cee-chen Aug 29, 2023 •

edited

Loading

kibanamachine commented Aug 28, 2023

1Copenut left a comment

1Copenut Aug 28, 2023

kibanamachine commented Aug 28, 2023

1Copenut left a comment

kibanamachine commented Aug 29, 2023

elasticmachine commented Aug 29, 2023

[Emotion][Theming] Add new consumer-configurable font.defaultUnits token #7133

[Emotion][Theming] Add new consumer-configurable font.defaultUnits token #7133

Conversation

cee-chen commented Aug 28, 2023 • edited Loading

Summary

Screencap of example usage:

QA

General checklist

kibanamachine commented Aug 28, 2023

cee-chen Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 28, 2023

Choose a reason for hiding this comment

cee-chen Aug 28, 2023

Choose a reason for hiding this comment

cee-chen Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

1Copenut Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

cee-chen Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Aug 28, 2023

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Aug 28, 2023

Choose a reason for hiding this comment

kibanamachine commented Aug 28, 2023

1Copenut left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 29, 2023

elasticmachine commented Aug 29, 2023

💚 Build Succeeded

History

[Emotion][Theming] Add new consumer-configurable `font.defaultUnits` token #7133

[Emotion][Theming] Add new consumer-configurable `font.defaultUnits` token #7133

cee-chen commented Aug 28, 2023 •

edited

Loading

cee-chen Aug 28, 2023 •

edited

Loading

cee-chen Aug 28, 2023 •

edited

Loading

1Copenut Aug 29, 2023 •

edited

Loading

cee-chen Aug 29, 2023 •

edited

Loading