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

Show file tree
Hide file tree
Changes from 8 commits
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
27 changes: 12 additions & 15 deletions src/components/facet/facet_button.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,33 +14,30 @@ import {
logicalTextAlignCSS,
} from '../../global_styling';

import { euiLinkFocusCSS } from '../link/link.styles';

export const euiFacetButtonStyles = ({ euiTheme }: UseEuiTheme) => ({
// Base
euiFacetButton: css`
${logicalTextAlignCSS('left')}

&:hover,
&:focus {
/* Make sure the quantity doesn't get an underline on hover */
&:not(:disabled) [class*='euiFacetButton__text'] {
text-decoration: underline;
&:not(:disabled) {
&:hover,
&:focus {
/* Make sure the quantity doesn't get an underline on hover */
text-decoration: none;

.euiFacetButton__text {
text-decoration: underline;
}
}
}

&:focus:not(:disabled) [class*='euiFacetButton__text'] {
${euiLinkFocusCSS(euiTheme)}
&:focus .euiFacetButton__text {
text-decoration-thickness: ${euiTheme.border.width.thick};
}
}

&:disabled {
color: ${euiTheme.colors.disabledText};
pointer-events: none;

&:hover,
&:focus {
text-decoration: none;
}
}
`,
});
Expand Down
9 changes: 6 additions & 3 deletions src/components/link/external_link_icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import React, { FunctionComponent, AnchorHTMLAttributes } from 'react';

import { useEuiTheme } from '../../services';
import { useEuiMemoizedStyles, UseEuiTheme } from '../../services';
import { logicalStyle } from '../../global_styling';
import { EuiIcon, EuiIconProps } from '../icon';
import { EuiI18n, useEuiI18n } from '../i18n';
Expand All @@ -28,10 +28,13 @@ export type EuiExternalLinkIconProps = {
external?: boolean;
};

const iconStyle = ({ euiTheme }: UseEuiTheme) =>
logicalStyle('margin-left', euiTheme.size.xs);

export const EuiExternalLinkIcon: FunctionComponent<
EuiExternalLinkIconProps & Partial<EuiIconProps>
> = ({ target, external, ...rest }) => {
const { euiTheme } = useEuiTheme();
const iconCssStyle = useEuiMemoizedStyles(iconStyle);

const showExternalLinkIcon =
(target === '_blank' && external !== false) || external === true;
Expand All @@ -45,7 +48,7 @@ export const EuiExternalLinkIcon: FunctionComponent<
<>
{showExternalLinkIcon && (
<EuiIcon
css={logicalStyle('margin-left', euiTheme.size.xs)}
css={iconCssStyle}
aria-label={iconAriaLabel}
size="s"
type="popout"
Expand Down
38 changes: 13 additions & 25 deletions src/components/link/link.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,42 +10,20 @@ import { css } from '@emotion/react';
import { UseEuiTheme } from '../../services';
import { euiFocusRing, logicalTextAlignCSS } from '../../global_styling';

const _colorCSS = (color: string) => {
return `
color: ${color};

&:target {
color: darken(${color}, 10%);
}
`;
};

export const euiLinkHoverCSS = () => {
return `
text-decoration: underline;
`;
};

export const euiLinkFocusCSS = (euiTheme: UseEuiTheme['euiTheme']) => {
return `
text-decoration: underline;
text-decoration-thickness: ${euiTheme.border.width.thick};
`;
};

export const euiLinkCSS = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;
return `
font-weight: ${euiTheme.font.weight.medium};
${logicalTextAlignCSS('left')}

&:hover {
${euiLinkHoverCSS()}
text-decoration: underline;
}

&:focus {
${euiFocusRing(euiThemeContext, 'outset')}
${euiLinkFocusCSS(euiTheme)}
text-decoration: underline;
text-decoration-thickness: ${euiTheme.border.width.thick};
}
`;
};
Expand Down Expand Up @@ -85,3 +63,13 @@ export const euiLinkStyles = (euiThemeContext: UseEuiTheme) => {
text: css(_colorCSS(euiTheme.colors.text)),
};
};

const _colorCSS = (color: string) => {
return `
color: ${color};

&:target {
color: darken(${color}, 10%);
}
`;
};
5 changes: 2 additions & 3 deletions src/components/link/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
} from 'react';
import classNames from 'classnames';

import { getSecureRelForTarget, useEuiTheme } from '../../services';
import { getSecureRelForTarget, useEuiMemoizedStyles } from '../../services';
import { CommonProps, ExclusiveUnion } from '../common';
import { validateHref } from '../../services/security/href_validator';

Expand Down Expand Up @@ -92,8 +92,7 @@ const EuiLink = forwardRef<HTMLAnchorElement | HTMLButtonElement, EuiLinkProps>(
},
ref
) => {
const euiTheme = useEuiTheme();
const styles = euiLinkStyles(euiTheme);
const styles = useEuiMemoizedStyles(euiLinkStyles);
const cssStyles = [styles.euiLink];

const isHrefValid = !href || validateHref(href);
Expand Down
5 changes: 2 additions & 3 deletions src/components/text/text.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import React, { FunctionComponent, HTMLAttributes, CSSProperties } from 'react';
import classNames from 'classnames';
import { CommonProps } from '../common';

import { useEuiTheme } from '../../services';
import { useEuiMemoizedStyles } from '../../services';
import { euiTextStyles } from './text.styles';

import { TextColor, EuiTextColor } from './text_color';
Expand Down Expand Up @@ -44,8 +44,7 @@ export const EuiText: FunctionComponent<EuiTextProps> = ({
className,
...rest
}) => {
const euiTheme = useEuiTheme();
const styles = euiTextStyles(euiTheme);
const styles = useEuiMemoizedStyles(euiTextStyles);
const cssStyles = [
styles.euiText,
!grow ? styles.constrainedWidth : undefined,
Expand Down
4 changes: 2 additions & 2 deletions src/components/text/text_align.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import { css } from '@emotion/react';
import { logicalTextAlignCSS } from '../../global_styling';

export const euiTextAlignStyles = () => ({
export const euiTextAlignStyles = {
euiTextAlign: css``,
left: css`
${logicalTextAlignCSS('left')}
Expand All @@ -20,4 +20,4 @@ export const euiTextAlignStyles = () => ({
center: css`
${logicalTextAlignCSS('center')}
`,
});
};
3 changes: 1 addition & 2 deletions src/components/text/text_align.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
import { CommonProps } from '../common';
import { cloneElementWithCss } from '../../services';

import { euiTextAlignStyles } from './text_align.styles';
import { euiTextAlignStyles as styles } from './text_align.styles';

export const ALIGNMENTS = ['left', 'right', 'center'] as const;
export type TextAlignment = (typeof ALIGNMENTS)[number];
Expand All @@ -35,7 +35,6 @@ export const EuiTextAlign: FunctionComponent<EuiTextAlignProps> = ({
cloneElement = false,
...rest
}) => {
const styles = euiTextAlignStyles();
const cssStyles = [styles.euiTextAlign, styles[textAlign]];

const props = { css: cssStyles, ...rest };
Expand Down
5 changes: 2 additions & 3 deletions src/components/text/text_color.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import React, {
} from 'react';

import { CommonProps } from '../common';
import { useEuiTheme, cloneElementWithCss } from '../../services';
import { useEuiMemoizedStyles, cloneElementWithCss } from '../../services';

import { euiTextColorStyles } from './text_color.styles';

Expand Down Expand Up @@ -60,8 +60,7 @@ export const EuiTextColor: FunctionComponent<EuiTextColorProps> = ({
}) => {
const isNamedColor = COLORS.includes(color as TextColor);

const euiTheme = useEuiTheme();
const styles = euiTextColorStyles(euiTheme);
const styles = useEuiMemoizedStyles(euiTextColorStyles);
const cssStyles = [
styles.euiTextColor,
isNamedColor ? styles[color as TextColor] : styles.customColor,
Expand Down
6 changes: 4 additions & 2 deletions src/components/title/title.styles.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,17 @@

import { renderHook } from '@testing-library/react';

import { useEuiTheme } from '../../services';

import { TITLE_SIZES } from './title';
import { useEuiTitle } from './title.styles';
import { euiTitle } from './title.styles';

describe('euiTitle mixin', () => {
describe('returns a static object of title font properties for each title size', () => {
TITLE_SIZES.forEach((size) => {
it(size, () => {
expect(
renderHook(() => useEuiTitle(size)).result.current
renderHook(() => euiTitle(useEuiTheme(), size)).result.current
).toMatchSnapshot();
});
});
Expand Down
11 changes: 1 addition & 10 deletions src/components/title/title.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { CSSProperties } from 'react';
import { css } from '@emotion/react';
import { UseEuiTheme, useEuiTheme } from '../../services';
import { UseEuiTheme } from '../../services';
import {
euiTextBreakWord,
euiFontSize,
Expand Down Expand Up @@ -56,15 +56,6 @@ export const euiTitle = (
};
};

// Hook version
export const useEuiTitle = (
scale: EuiTitleSize,
options?: _FontScaleOptions
): EuiThemeTitle => {
const euiTheme = useEuiTheme();
return euiTitle(euiTheme, scale, options);
};

/**
* Styles
*/
Expand Down
9 changes: 5 additions & 4 deletions src/components/title/title.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@

import { FunctionComponent, ReactElement } from 'react';
import classNames from 'classnames';
import { useEuiTheme, cloneElementWithCss } from '../../services';
import { euiTitleStyles } from './title.styles';

import { useEuiMemoizedStyles, cloneElementWithCss } from '../../services';
import { CommonProps } from '../common';

import { euiTitleStyles } from './title.styles';

export const TITLE_SIZES = ['xxxs', 'xxs', 'xs', 's', 'm', 'l'] as const;
export type EuiTitleSize = (typeof TITLE_SIZES)[number];

Expand All @@ -35,8 +37,7 @@ export const EuiTitle: FunctionComponent<EuiTitleProps> = ({
textTransform,
...rest
}) => {
const euiTheme = useEuiTheme();
const styles = euiTitleStyles(euiTheme);
const styles = useEuiMemoizedStyles(euiTitleStyles);
const cssStyles = [
styles.euiTitle,
textTransform ? styles[textTransform] : undefined,
Expand Down
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