Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Aug 29, 2023
23 changes: 22 additions & 1 deletion src-docs/src/views/theme/typography/_typography_js.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { FunctionComponent, useState } from 'react';
import { css } from '@emotion/react';
import { useEuiTheme } from '../../../../../src/services';
import { useEuiTheme, EuiThemeProvider } from '../../../../../src/services';
import {
EuiBasicTable,
EuiButtonGroup,
Expand All @@ -9,6 +9,7 @@ import {
EuiDescribedFormGroup,
EuiPanel,
EuiSpacer,
EuiText,
} from '../../../../../src/components';

import {
Expand Down Expand Up @@ -104,6 +105,26 @@ export const FontJS = () => {
snippet={'font-feature-settings: ${euiTheme.font.featureSettings};'}
snippetLanguage="emotion"
/>

<ThemeExample
title={<code>euiTheme.font.fontSizeUnit</code>}
description={getDescription(baseProps.fontSizeUnit)}
example={
<EuiThemeProvider modify={{ font: { fontSizeUnit: 'px' } }}>
<EuiText>
My font size and line height is set using <EuiCode>px</EuiCode>{' '}
and not <EuiCode>rem</EuiCode>
</EuiText>
</EuiThemeProvider>
}
snippet={`<EuiProvider modify={{ font: { fontSizeUnit: 'px' } }}>
<EuiText>
<p>Hello world</p>
</EuiText>
</EuiProvider>
`}
snippetLanguage="jsx"
/>
</>
);
};
Expand Down
3 changes: 0 additions & 3 deletions src/components/markdown_editor/markdown_format.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,16 @@ export const euiMarkdownFormatStyles = (euiTheme: UseEuiTheme) => ({
// Text sizes
m: css`
${euiScaleMarkdownFormatText(euiTheme, {
measurement: 'rem',
customScale: 'm',
})}
`,
s: css`
${euiScaleMarkdownFormatText(euiTheme, {
measurement: 'rem',
customScale: 's',
})}
`,
xs: css`
${euiScaleMarkdownFormatText(euiTheme, {
measurement: 'rem',
customScale: 'xs',
})}
`,
Expand Down
3 changes: 0 additions & 3 deletions src/components/text/text.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -345,19 +345,16 @@ export const euiTextStyles = (euiThemeContext: UseEuiTheme) => {
// Sizes
m: css`
${euiScaleText(euiThemeContext, {
measurement: 'rem',
customScale: 'm',
})}
`,
s: css`
${euiScaleText(euiThemeContext, {
measurement: 'rem',
customScale: 's',
})}
`,
xs: css`
${euiScaleText(euiThemeContext, {
measurement: 'rem',
customScale: 'xs',
})}
`,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`euiFontSizeFromScale measurement em 1`] = `"1em"`;

exports[`euiFontSizeFromScale measurement px 1`] = `"16px"`;

exports[`euiFontSizeFromScale measurement rem 1`] = `"1.1429rem"`;

exports[`euiFontSizeFromScale scale l 1`] = `"1.5714rem"`;

exports[`euiFontSizeFromScale scale m 1`] = `"1.1429rem"`;

exports[`euiFontSizeFromScale scale s 1`] = `"1.0000rem"`;

exports[`euiFontSizeFromScale scale xl 1`] = `"1.9286rem"`;

exports[`euiFontSizeFromScale scale xs 1`] = `"0.8571rem"`;

exports[`euiFontSizeFromScale scale xxl 1`] = `"2.4286rem"`;

exports[`euiFontSizeFromScale scale xxs 1`] = `"0.7857rem"`;

exports[`euiFontSizeFromScale scale xxxs 1`] = `"0.6429rem"`;

exports[`euiLineHeightFromBaseline measurement em 1`] = `"1.5000"`;

exports[`euiLineHeightFromBaseline measurement px 1`] = `"24px"`;

exports[`euiLineHeightFromBaseline measurement rem 1`] = `"1.7143rem"`;

exports[`euiLineHeightFromBaseline scale l 1`] = `"1.7143rem"`;

exports[`euiLineHeightFromBaseline scale m 1`] = `"1.7143rem"`;

exports[`euiLineHeightFromBaseline scale s 1`] = `"1.4286rem"`;

exports[`euiLineHeightFromBaseline scale xl 1`] = `"2.2857rem"`;

exports[`euiLineHeightFromBaseline scale xs 1`] = `"1.1429rem"`;

exports[`euiLineHeightFromBaseline scale xxl 1`] = `"2.8571rem"`;

exports[`euiLineHeightFromBaseline scale xxs 1`] = `"1.1429rem"`;

exports[`euiLineHeightFromBaseline scale xxxs 1`] = `"0.8571rem"`;
114 changes: 114 additions & 0 deletions src/global_styling/functions/typography.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import React, { FunctionComponent, PropsWithChildren } from 'react';
import { renderHook } from '@testing-library/react';

import { EuiProvider } from '../../components';
import { useEuiTheme } from '../../services';
import {
EuiThemeFontScales,
EuiThemeFontSizeMeasurements,
} from '../variables/typography';

import { euiFontSizeFromScale, euiLineHeightFromBaseline } from './typography';

// Default euiTheme to use for most tests
const { euiTheme } = renderHook(useEuiTheme).result.current;

describe('euiFontSizeFromScale', () => {
describe('scale', () => {
EuiThemeFontScales.forEach((scale) => {
test(scale, () => {
expect(euiFontSizeFromScale(scale, euiTheme)).toMatchSnapshot();
});
});
});

describe('custom scale', () => {
it('allows passing a custom modifier to the existing scale', () => {
expect(
euiFontSizeFromScale('xxxs', euiTheme, { customScale: 's' })
).toEqual('0.5625rem');
});
});

describe('measurement', () => {
EuiThemeFontSizeMeasurements.forEach((unit) => {
test(unit, () => {
const output = euiFontSizeFromScale('m', euiTheme, {
measurement: unit,
});
expect(output.endsWith(unit)).toBe(true);
expect(output).toMatchSnapshot();
});
});

it('falls back to the `fontSizeUnit` theme token if measurement is not passed', () => {
const wrapper: FunctionComponent<PropsWithChildren> = ({ children }) => (
<EuiProvider modify={{ font: { fontSizeUnit: 'px' } }}>
{children}
</EuiProvider>
);
const { euiTheme: modifiedEuiTheme } = renderHook(useEuiTheme, {
wrapper,
}).result.current;

expect(euiFontSizeFromScale('m', modifiedEuiTheme)).toEqual('16px');
});
});
});

describe('euiLineHeightFromBaseline', () => {
describe('scale', () => {
EuiThemeFontScales.forEach((scale) => {
test(scale, () => {
expect(euiLineHeightFromBaseline(scale, euiTheme)).toMatchSnapshot();
});
});
});

describe('custom scale', () => {
it('allows passing a custom modifier to the existing scale', () => {
expect(
euiLineHeightFromBaseline('xxxs', euiTheme, { customScale: 's' })
).toEqual('0.8571rem');
});
});

describe('measurement', () => {
EuiThemeFontSizeMeasurements.forEach((unit) => {
test(unit, () => {
const output = euiLineHeightFromBaseline('m', euiTheme, {
measurement: unit,
});

if (unit !== 'em') {
expect(output.endsWith(unit)).toBe(true);
} else {
expect(Number(output)).not.toBeNaN();
}

expect(output).toMatchSnapshot();
});
});

it('falls back to the `fontSizeUnit` theme token if measurement is not passed', () => {
const wrapper: FunctionComponent<PropsWithChildren> = ({ children }) => (
<EuiProvider modify={{ font: { fontSizeUnit: 'px' } }}>
{children}
</EuiProvider>
);
const { euiTheme: modifiedEuiTheme } = renderHook(useEuiTheme, {
wrapper,
}).result.current;

expect(euiLineHeightFromBaseline('m', modifiedEuiTheme)).toEqual('24px');
});
});
});
4 changes: 2 additions & 2 deletions src/global_styling/functions/typography.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export interface _FontScaleOptions {
export function euiFontSizeFromScale(
scale: _EuiThemeFontScale,
{ base, font }: UseEuiTheme['euiTheme'],
{ measurement = 'rem', customScale }: _FontScaleOptions = {}
{ measurement = font.fontSizeUnit, customScale }: _FontScaleOptions = {}
) {
if (measurement === 'em') {
return `${font.scale[scale]}em`;
Expand Down Expand Up @@ -68,7 +68,7 @@ export function euiFontSizeFromScale(
export function euiLineHeightFromBaseline(
scale: _EuiThemeFontScale,
{ base, font }: UseEuiTheme['euiTheme'],
{ measurement = 'rem', customScale }: _FontScaleOptions = {}
{ measurement = font.fontSizeUnit, customScale }: _FontScaleOptions = {}
) {
const { baseline, lineHeightMultiplier } = font;
let numerator = base * font.scale[scale];
Expand Down
9 changes: 9 additions & 0 deletions src/global_styling/variables/typography.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@ export type _EuiThemeFontBase = {
* https://developer.mozilla.org/en-US/docs/Web/CSS/font-feature-settings
*/
featureSettings?: string;
/**
* Sets the default font size unit used by UI components like EuiText or EuiTitle.
*
* NOTE: This may overridden by some internal usages, e.g.
* EuiText's `relative` size which must use `em`.
*
* @default 'rem'
*/
fontSizeUnit: _EuiThemeFontSizeMeasurement;
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
/**
* A computed number that is 1/4 of `base`
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

@cee-chen cee-chen Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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',

??

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@cee-chen cee-chen Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

@1Copenut 1Copenut Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@cee-chen cee-chen Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


baseline: computed(([base]) => base / 4, ['base']),
lineHeightMultiplier: 1.5,
Expand Down
1 change: 1 addition & 0 deletions upcoming_changelogs/7133.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Added `font.fontSizeUnit` theme token. EUI component font sizes default to `rem` units - this token allows consumers to configure this to `px` or `em`
Loading