Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions packages/eui/src/components/card/card.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import { css } from '@emotion/react';

import {
EuiPaddingSize,
euiPaddingSize,
logicalCSS,
logicalTextAlignCSS,
Expand Down Expand Up @@ -199,6 +200,19 @@ export const euiCardStyles = (
};
};

// Static per-size generators so `useEuiMemoizedStyles` can share them across instances
export const euiCardStylesByPadding: Record<

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💭 Non blocking, general thought: We're creating 6 cached style objects that are in large parts identical (only image/icon use paddingAmount). If we wanted to optimize further we could split shared styles and dynamic styles into separate memoized chunks. It reduces duplicate cached styles but adds a bit more complexity in code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mgadewoll, I wanted to avoid changing the generated classes and markup to avoid jumping hurdles with the integration branch and a large EUI PR review :D Do you think it is a wrong approach to optimize for that in this case? I so, we can adjut the code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't have to update anything additional in this PR. I just think we might want to ask what the end state of the style memoization should be: Are we fine with duplicated cached styles or do we opt to optimize that as well and is it worth it. That could be a separate update if we want to do it.

Just for transparency: the split wouldn't change any classes afaik, I'm thinking of something like this:

// card.tsx
const baseStyles = useEuiMemoizedStyles(euiCardStyles);
const paddingStyles = useEuiMemoizedStyles(
  euiCardStylesByPadding[paddingSize]
);
const styles = { ...baseStyles, ...paddingStyles };
  
// card.styles.ts
export const euiCardStyles = (euiThemeContext: UseEuiTheme) => {
  const { euiTheme } = euiThemeContext;
  const spacing = euiPaddingSize(euiThemeContext, paddingKey);
  const halfSpacing = euiPaddingSize(euiThemeContext, halfPaddingKey);

  return {
    card: {}
    ...
    // no image or icon styles
  }
}

export const euiCardDynamicStyles = (
  euiThemeContext: UseEuiTheme,
  paddingSize: EuiCardProps['paddingSize']
) => {
  const { euiTheme } = euiThemeContext;
  const paddingAmount = euiPaddingSize(euiThemeContext, paddingSize!);
  const spacing = euiPaddingSize(euiThemeContext, paddingKey);
  const halfSpacing = euiPaddingSize(euiThemeContext, halfPaddingKey);

  return {
    image: {
      euiCard__image: css`
        position: relative;
        overflow: hidden;

        /* Padding based sizing & negative margins */
        ${logicalCSS('width', `calc(100% + (${paddingAmount} * 2))`)}
        ${logicalCSS('left', `-${paddingAmount}`)}
        ${logicalCSS('top', `-${paddingAmount}`)}
        /* Ensure the parent is only as tall as the image */
        ${logicalCSS('margin-bottom', `-${paddingAmount}`)}

        /* Match border radius */
        ${logicalCSS(
          'border-top-left-radius',
          `calc(${euiTheme.border.radius.panel})`
        )}
        ${logicalCSS(
          'border-top-right-radius',
          `calc(${euiTheme.border.radius.panel})`
        )};

        img {
          ${logicalCSS('width', '100%')}/* 4 */
        }
      `,
      transparent: css`
        border-radius: ${euiTheme.border.radius.panel};
      `,
    },

    icon: {
      euiCard__icon: css``,

      withImage: css`
        position: absolute;
        ${logicalCSS('top', '50%')}
        ${logicalCSS('left', '50%')}
        /* Important needed to override current Sass styles on .euiIcon */
        transform: translate(
          -50%,
          calc(-50% + -${paddingAmount})
        ) !important; /* stylelint-disable-line declaration-no-important */
      `,

      layout: {
        vertical: css`
          ${logicalCSS('margin-top', halfSpacing)}
        `,
        horizontal: css`
          ${logicalCSS('margin-right', spacing)}
        `,
      },
    },
  };
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, i see! I guess these would worth it if a component has a lot of variations that are widely used sizes × colors × …

EuiPaddingSize,
(euiThemeContext: UseEuiTheme) => ReturnType<typeof euiCardStyles>
> = {
none: (euiThemeContext) => euiCardStyles(euiThemeContext, 'none'),
xs: (euiThemeContext) => euiCardStyles(euiThemeContext, 'xs'),
s: (euiThemeContext) => euiCardStyles(euiThemeContext, 's'),
m: (euiThemeContext) => euiCardStyles(euiThemeContext, 'm'),
l: (euiThemeContext) => euiCardStyles(euiThemeContext, 'l'),
xl: (euiThemeContext) => euiCardStyles(euiThemeContext, 'xl'),
};

export const euiCardTextStyles = (euiThemeContext: UseEuiTheme) => {
const { euiTheme } = euiThemeContext;
return {
Expand Down
7 changes: 4 additions & 3 deletions packages/eui/src/components/card/card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import classNames from 'classnames';
import {
getSecureRelForTarget,
useEuiTheme,
useEuiMemoizedStyles,
cloneElementWithCss,
} from '../../services';
import { useGeneratedHtmlId } from '../../services/accessibility';
Expand All @@ -34,7 +35,7 @@ import { EuiSpacer } from '../spacer';
import { EuiCardSelect, EuiCardSelectProps } from './card_select';
import {
euiCardBetaBadgeStyles,
euiCardStyles,
euiCardStylesByPadding,
euiCardTextStyles,
} from './card.styles';

Expand Down Expand Up @@ -170,7 +171,7 @@ export const EuiCard: FunctionComponent<EuiCardProps> = ({
!isDisabled && (onClick || href || (selectable && !selectable.isDisabled));

const euiThemeContext = useEuiTheme();
const styles = euiCardStyles(euiThemeContext, paddingSize);
const styles = useEuiMemoizedStyles(euiCardStylesByPadding[paddingSize]);
const cardStyles = [
styles.card.euiCard,
// Text alignment should always be left when horizontal
Expand All @@ -185,7 +186,7 @@ export const EuiCard: FunctionComponent<EuiCardProps> = ({

const mainStyles = [styles.main.euiCard__main, styles.main.layout[layout]];

const textStyles = euiCardTextStyles(euiThemeContext);
const textStyles = useEuiMemoizedStyles(euiCardTextStyles);
const textCSS = [
textStyles.euiCard__text,
// Text alignment should always be left when horizontal
Expand Down
2 changes: 1 addition & 1 deletion packages/eui/src/components/code/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,10 @@ function wrapLines(
}
});
const wrapped: RefractorNode[] = [];
const styles = euiCodeBlockLineStyles(euiTheme);
grouped.forEach((node, i) => {
let children: RefractorNode[] = node;

const styles = euiCodeBlockLineStyles(euiTheme);
const lineStyles = cx([
styles.euiCodeBlock__line,
options.showLineNumbers && styles.hasLineNumbers,
Expand Down
6 changes: 6 additions & 0 deletions packages/eui/src/components/progress/progress.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,12 @@ export const euiProgressStyles = (
/**
* Data styles
*/
// Static per-variant generators so `useEuiMemoizedStyles` can share them across instances
export const euiProgressNativeStyles = (euiThemeContext: UseEuiTheme) =>
euiProgressStyles(euiThemeContext, true);
export const euiProgressIndeterminateStyles = (euiThemeContext: UseEuiTheme) =>
euiProgressStyles(euiThemeContext, false);

export const euiProgressDataStyles = (euiThemeContext: UseEuiTheme) => ({
euiProgress__data: css`
display: flex;
Expand Down
17 changes: 12 additions & 5 deletions packages/eui/src/components/progress/progress.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@ import { EuiI18n } from '../i18n';
import { CommonProps, ExclusiveUnion } from '../common';
import { isNil } from '../../services/predicate';

import { useEuiTheme, makeHighContrastColor } from '../../services';
import {
useEuiTheme,
useEuiMemoizedStyles,
makeHighContrastColor,
} from '../../services';
import { EuiScreenReaderOnly } from '../accessibility';
import {
euiProgressStyles,
euiProgressNativeStyles,
euiProgressIndeterminateStyles,
euiProgressDataStyles,
euiProgressLabelStyles,
euiProgressValueTextStyles,
Expand Down Expand Up @@ -120,7 +125,9 @@ export const EuiProgress: FunctionComponent<
? { color: makeHighContrastColor(color)(euiTheme.euiTheme) }
: {};

const styles = euiProgressStyles(euiTheme, determinate);
const styles = useEuiMemoizedStyles(
determinate ? euiProgressNativeStyles : euiProgressIndeterminateStyles
);
Comment on lines +128 to +130

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is not important. the boost comes from expensive makeHighContrastColor and the native/indeterminate blocks

const cssStyles = [
styles.euiProgress,
determinate && styles.native,
Expand All @@ -130,7 +137,7 @@ export const EuiProgress: FunctionComponent<
isNamedColor ? styles[color as EuiProgressColor] : styles.customColor,
];

const dataStyles = euiProgressDataStyles(euiTheme);
const dataStyles = useEuiMemoizedStyles(euiProgressDataStyles);
const dataCssStyles = [
dataStyles.euiProgress__data,
size === 'l' && dataStyles[size],
Expand All @@ -139,7 +146,7 @@ export const EuiProgress: FunctionComponent<
euiProgressLabelStyles.euiProgress__label,
labelProps?.css,
];
const valueTextStyles = euiProgressValueTextStyles(euiTheme);
const valueTextStyles = useEuiMemoizedStyles(euiProgressValueTextStyles);
const valueTextCssStyles = [
valueTextStyles.euiProgress__valueText,
isNamedColor
Expand Down
10 changes: 10 additions & 0 deletions packages/eui/src/components/token/token.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,13 @@ export const euiTokenStyles = (
none: css``,
};
};

// Static per-fill generators so `useEuiMemoizedStyles` can share them across instances
export const euiTokenFillStyles: Record<
TokenFill,
(euiThemeContext: UseEuiTheme) => ReturnType<typeof euiTokenStyles>
> = {
light: (euiThemeContext) => euiTokenStyles(euiThemeContext, 'light'),
dark: (euiThemeContext) => euiTokenStyles(euiThemeContext, 'dark'),
none: (euiThemeContext) => euiTokenStyles(euiThemeContext, 'none'),
};
15 changes: 6 additions & 9 deletions packages/eui/src/components/token/token.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
* Side Public License, v 1.
*/

import React, { FunctionComponent, useMemo } from 'react';
import React, { FunctionComponent } from 'react';
import classNames from 'classnames';
import { useEuiTheme, isColorDark, hexToRgb } from '../../services';
import { useEuiMemoizedStyles, isColorDark, hexToRgb } from '../../services';

import { EuiIcon, IconSize } from '../icon';
import { EuiTokenMapType, TOKEN_MAP } from './token_map';
Expand All @@ -20,7 +20,7 @@ import type {
TokenShape,
TokenFill,
} from './token_types';
import { euiTokenStyles } from './token.styles';
import { euiTokenFillStyles } from './token.styles';

const isTokenColor = (color: string): color is TokenColor =>
COLORS.includes(color as TokenColor);
Expand Down Expand Up @@ -53,8 +53,6 @@ export const EuiToken: FunctionComponent<EuiTokenProps> = ({
finalSize = 'm';
}

const euiTheme = useEuiTheme();

// If the iconType passed is one of the prefab token types,
// grab its properties
const tokenDefaults =
Expand All @@ -66,10 +64,9 @@ export const EuiToken: FunctionComponent<EuiTokenProps> = ({
const finalShape = shape || tokenDefaults.shape || 'circle';
let finalFill = fill || 'light';

// memoize styles to reduce executing contained color calculations
const styles = useMemo(() => {
return euiTokenStyles(euiTheme, finalFill);
}, [euiTheme, finalFill]);
const styles = useEuiMemoizedStyles(
euiTokenFillStyles[finalFill as TokenFill]
);

let cssStyles = [
styles.euiToken,
Expand Down
Loading