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.

Sign up for GitHub

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] Memoize typography styles & mixins #7543

Merged
merged 9 commits into from
Mar 7, 2024
Merged

[Emotion] Memoize typography styles & mixins #7543

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7063808
[EuiText][EuiTextColor] Memoize styles
cee-chen Feb 23, 2024
bb3e7d9
[EuiTextAlign] Remove unnecessary styles function
cee-chen Feb 23, 2024
2da0324
[EuiTitle] Memoize styles
cee-chen Feb 23, 2024
4348b1d
[EuiTitle] Remove unused `useEuiTitle` hook/export
cee-chen Feb 23, 2024
21e68b0
[EuiLink] Memoize styles
cee-chen Feb 23, 2024
8753ca1
[EuiLink] Remove unnecessary style exports
cee-chen Feb 23, 2024
33e8448
[EuiFacetButton] Remove `euiLinkFocusCSS` usage
cee-chen Feb 23, 2024
04da2e6
[mixins] Memoize typography hooks
cee-chen Feb 23, 2024
f9242a8
[PR discussion] Remove `useEuiNumberFormat`
cee-chen Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 27 additions & 7 deletions src/global_styling/mixins/_typography.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,15 @@ import {
euiFontSizeFromScale,
_FontScaleOptions,
} from '../functions/typography';
import { useEuiTheme, UseEuiTheme } from '../../services/theme/hooks';
import { _EuiThemeFontScale } from '../variables/typography';
import {
useEuiMemoizedStyles,
useEuiTheme,
UseEuiTheme,
} from '../../services/theme';
import {
_EuiThemeFontScale,
EuiThemeFontScales,
} from '../variables/typography';
import { logicalCSS } from '../functions';

export type EuiThemeFontSize = {
Expand All @@ -39,7 +46,22 @@ export const useEuiFontSize = (
options?: _FontScaleOptions
): EuiThemeFontSize => {
const euiTheme = useEuiTheme();
return euiFontSize(euiTheme, scale, options);
const memoizedFontSizes = useEuiMemoizedStyles(euiFontSizes);

return !options
? memoizedFontSizes[scale]
: euiFontSize(euiTheme, scale, options);
};
// Memomize a basic set of font sizes. We unfortunately can't
// memoize all possible options, there's too many permutations
const euiFontSizes = (euiThemeContext: UseEuiTheme) => {
return EuiThemeFontScales.reduce(
(map, scale) => ({
...map,
[scale]: euiFontSize(euiThemeContext, scale),
}),
{} as Record<_EuiThemeFontScale, EuiThemeFontSize>
);
};

/**
Expand Down Expand Up @@ -71,7 +93,5 @@ export const euiTextTruncate = (
export const euiNumberFormat = ({ euiTheme }: UseEuiTheme) => `
font-feature-settings: ${euiTheme.font.featureSettings}, 'tnum' 1;
`;
export const useEuiNumberFormat = (): string => {
const euiTheme = useEuiTheme();
return euiNumberFormat(euiTheme);
};
export const useEuiNumberFormat = (): string =>
useEuiMemoizedStyles<any>(euiNumberFormat);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we could prevent using an any type here?
Could it make sense to enhance useEuiMemoizedStyles to return StylesMaps | string to support cases like useEuiNumberFormat? 🤔

type MemoizedStylesMap = WeakMap<Function, StylesMaps | string>;

const useEuiMemoizedStyles = <
  T extends (theme: UseEuiTheme) => StylesMaps | string
>(
  styleGenerator: T
): ReturnType<T> => {}

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how I feel about this. These utility functions return strings on purpose, and they should only be used in the css tagged template literal so that they can get processed, validated and injected in the runtime.

The only reason for them to be memoized is if they're passed directly to the css prop, not wrapped like this

css={css`
  ${useEuiNumberFormat()}
}`

because in this case, when the component rerenders, the styles will be recalculated anyway and memoization won't help much.

The css prop doesn't support passing a raw style string even though the Interpolation type allows it.

I believe that memoization here is unnecessary, and the responsibility to memoize customer styles lies on the consumer. If our goal is to provide one-line emotion utilities that are also memoized, we should probably also export useEuiNumberFormatCss() that does that exact thing and is meant to be used when the only thing you need is that single style (e.g., <p css={useEuiNumberFormatCss()}></p>)

Copy link
Contributor Author

@cee-chen cee-chen Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if we could prevent using an any type here?

see my question here about where my Styles type was too restrictive 🥲 #7529 (comment)

In light of this conversation I think it was, and I should modify it.

These utility functions return strings on purpose

I don't know if I agree with that and I don't think our architecture was meant to be that deliberate to be honest with you. Some utilities return strings so they don't trigger additional appended Emotion classnames, some use the css template literal because they do want to insert additional classNames. TBH, I don't think Caroline really had a specific rhyme or reason for it.

Also, our Emotion useX hooks are also fairly arbitrary and not well planned out, they currently have zero usage outside of our own documentation and to be frank: I don't like the idea of maintaining them. I'd rather just get rid of them entirely if we have the option.

Copy link
Member

Choose a reason for hiding this comment

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

Also, our Emotion useX hooks are also fairly arbitrary and not well planned out, they currently have zero usage outside of our own documentation and to be frank: I don't like the idea of maintaining them. I'd rather just get rid of them entirely if we have the option.

In that case, maybe it's not worth adding memoization to them since the outputs of these functions vary, and they aren't really used externally. There's no need for adding memoization to small and rarely used utility styles, especially if we aren't sure we want to keep them in the code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that, I'd also vote to just rip out useEuiNumberFormat in this PR / in our docs if no one objects. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f9242a8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkajtoch is this PR good to merge on your end or do you have any further feedback/change requests?

Loading