Skip to content
Closed
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
36 changes: 36 additions & 0 deletions packages/eui/src/components/panel/panel.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,15 @@

import React from 'react';
import type { Meta, StoryObj } from '@storybook/react';
import { css } from '@emotion/react';

import {
disableStorybookControls,
enableFunctionToggleControls,
} from '../../../.storybook/utils';
import { EuiPanel, EuiPanelProps } from './panel';
import { EuiButton } from '../button';
import { EuiFlexGroup } from '../flex';

const meta: Meta<EuiPanelProps> = {
title: 'Layout/EuiPanel',
Expand Down Expand Up @@ -74,3 +77,36 @@ export const OverlappingPanels: Story = {
);
},
};

const panelStyles = (isPrimary: boolean) => {
return css`
background-color: ${isPrimary ? 'blue' : 'gray'};
Copy link
Member

Choose a reason for hiding this comment

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

This is a good example of a technically dynamic style that may or may not be harmful. If it changes too frequently, it might, but overall, this example is okay. It's more of a question of how we want users to define styles specific to component props (or other flags and settings). I usually prefer to follow BEM-like structure and treat props as modifiers to primary component styles, but that's not always handy - it all depends on the structure of the styles (e.g., if you need to style child classes you don't control, this might be your only solid solution).

Generally speaking, we can recommend that Kibana should use BEM-like modifier classes whenever dealing with props affecting component styles, but that's just internal. I wonder if it makes a lot of a difference considering the switch to speedy being enabled by default there (and possibly becoming the default for EUI soon).

Do you know how does this detection work with Emotion's cssPropOptimization?

[Slightly off-topic]

We need to benchmark if there's a difference between:

  1. A single style with dynamically changing value
  2. An array of styles which we compose based on props
  3. Using style the normal way
  4. Using style or setProperty to pass a local CSS variable to use in the regular styles and letting browsers optimize it to reduce the number of style calculations and paints (I don't think this will be as performant as it sounds, though)

Without any data, I'd say that:

  • A single style with a dynamically changing value will take some time to recompute all styles when any input value changes. This is usually very fast but could easily take 20ms for large stylesheets
  • An array of styles could theoretically only recompute the stylesheet with just the single value (property) changing. This would be faster than recomputing the whole stylesheet, but it would also have to recreate the styles array and would be heavily dependent on other styles' caching which also has some resource cost associated with it
  • style attribute updates will cause a rerender and repaint, similar to element.style.setProperty
  • Even if just a single class name changes, the element will need to have its styles recomputed and will likely cause a rerender. I'm not aware of any browser optimizations that would not rerender unless the new class names point to styles that are exactly the same

color: white;
padding: 16px;
border-radius: 4px;
`;
};

export const DynamicStylePanel: Story = {
args: {
children: 'Click the button to change panel styles',
},
render: function Render(args: EuiPanelProps) {
const [isPrimary, setIsPrimary] = React.useState(false);

return (
<EuiFlexGroup alignItems="baseline" direction="column" gutterSize="s">
<EuiButton onClick={() => setIsPrimary((v) => !v)}>
Toggle dynamic style
</EuiButton>
<EuiPanel
{...args}
color={isPrimary ? 'primary' : 'plain'}
css={panelStyles(isPrimary)}
>
{args.children}
</EuiPanel>
</EuiFlexGroup>
);
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import React, { PropsWithChildren } from 'react';
import { EmotionCache } from '@emotion/css';
import { CacheProvider } from '@emotion/react';

import { patchCacheForDynamicStyles } from './emotion_dynamic_patch';

export interface EuiCacheProviderProps extends PropsWithChildren {
cache?: false | EmotionCache;
}
Expand All @@ -18,6 +20,13 @@ export const EuiCacheProvider = ({
cache,
children,
}: EuiCacheProviderProps) => {
if (
cache &&
process.env.NODE_ENV !== 'production' &&
process.env.NODE_ENV !== 'test'
) {
patchCacheForDynamicStyles(cache);
}
return children && cache ? (
<CacheProvider value={cache}>{children}</CacheProvider>
) : (
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* 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 { patchCacheForDynamicStyles } from './emotion_dynamic_patch';

describe('patchCacheForDynamicStyles', () => {
function createMockCache() {
const inserted: any[] = [];
return {
insert: jest.fn(function (selector, serialized, sheet, shouldCache) {
inserted.push({ selector, serialized, sheet, shouldCache });
return undefined;
}),
inserted,
};
}

it('should not warn for static styles', () => {
const cache: any = createMockCache();
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
patchCacheForDynamicStyles(cache);

// Simulate two static style insertions with the same base class and hash
cache.insert('.css-aaaaaa-euiPanel', {
name: 'aaaaaa-euiPanel',
styles: 'color:red;',
});
cache.insert('.css-aaaaaa-euiPanel', {
name: 'aaaaaa-euiPanel',
styles: 'color:red;',
});

expect(warnSpy).not.toHaveBeenCalled();
warnSpy.mockRestore();
});

it('should warn for dynamic styles (different hashes for same base)', () => {
const cache: any = createMockCache();
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
patchCacheForDynamicStyles(cache);

// Simulate two dynamic style insertions with different hashes but same base class
cache.insert('.css-aaaaaa-euiPanel', {
name: 'aaaaaa-euiPanel',
styles: 'color:red;',
});
cache.insert('.css-bbbbbb-euiPanel', {
name: 'bbbbbb-euiPanel',
styles: 'color:blue;',
});

expect(warnSpy).toHaveBeenCalledWith(
expect.stringContaining(
'Dynamic style detected for base selector: euiPanel'
)
);
warnSpy.mockRestore();
});

it('should handle selectors that do not match the pattern', () => {
const cache: any = createMockCache();
const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {});
patchCacheForDynamicStyles(cache);

// Insert a selector that doesn't match the .css-xxxxxx-base pattern
cache.insert('.not-css-selector', {
name: 'notcssselector',
styles: 'color:green;',
});

expect(warnSpy).not.toHaveBeenCalled();
warnSpy.mockRestore();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* 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 { EmotionCache } from '@emotion/react';

/**
* Applies a monkey patch to the given Emotion cache to flag dynamic styles.
* This is modular and can be reused in other places.
*/
export function patchCacheForDynamicStyles(cache: EmotionCache) {
Copy link
Member

Choose a reason for hiding this comment

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

This would be a good place to use the decorator pattern

if (!cache || typeof cache !== 'object' || (cache as any).__patched) return;

const baseClassMap = new Map<string, Set<string>>();

/**
* A utility to normalize selectors by removing the dynamic part
* of the class name generated by Emotion.
*/
function getBaseClass(selector: string): string | null {
Copy link
Member

Choose a reason for hiding this comment

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

Please note that this implementation is configuration-sensitive and may break when a custom labelFormat is used

Copy link
Contributor

Choose a reason for hiding this comment

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

💭 The regex only works with the base cache setup, which is by default named as starting with "css". If a cache is created with createCache which requires a custom key (code) and that key is not "css" then this matching will not work.
We could consider exporting a set of cache names that are matched against to limit the risk. Or provide means to define the cache names.

Copy link
Contributor Author

@weronikaolejniczak weronikaolejniczak Jun 17, 2025

Choose a reason for hiding this comment

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

Yup, it is mentioned in one of the limitations in the description. The list of cache names could be limiting but is an idea we can explore.

const match = selector.match(/\.css-[a-zA-Z0-9]+-(.+)/);
return match ? match[1] : null;
}

/**
* Checks for dynamic styles by tracking hashes for each base class.
* If multiple hashes are seen for the same base class, a warning is logged.
* Returns true if a dynamic style was flagged.
*/
function flagDynamicStyles(selector: string, serialized: any): boolean {
const baseClass = getBaseClass(selector);

if (baseClass) {
const hashes = baseClassMap.get(baseClass) || new Set<string>();
const hash =
serialized && serialized.name ? serialized.name.split('-')[0] : null;

if (hash) {
hashes.add(hash);
if (hashes.size > 1) {
console.warn(
Copy link
Member

Choose a reason for hiding this comment

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

The warning should likely be emitted only once per style to reduce console spam in cases of frequently updated styles (e.g., related to scroll positions).

`[EuiCacheProvider] Dynamic style detected for base selector: ${baseClass}. ` +
'This usually means you are passing dynamic values to the `css` prop. ' +
'Consider using the `style` prop for dynamic values instead.'
);
baseClassMap.set(baseClass, hashes);

return true;
}

baseClassMap.set(baseClass, hashes);
}
}
return false;
}

const originalInsert = cache.insert;
cache.insert = function (selector, serialized, sheet, shouldCache) {
flagDynamicStyles(selector, serialized);
return originalInsert.call(this, selector, serialized, sheet, shouldCache);
};
(cache as any).__patched = true;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this flag at all?

(cache as any).__baseClassMap = baseClassMap;
}