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

Jump to bottom

[Emotion] Add Emotion theming support #6913

Merged
merged 7 commits into from
Jul 12, 2023
Merged

[Emotion] Add Emotion theming support #6913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e287afe
[setup] Move dev provider warnings to its own file
cee-chen Jul 6, 2023
ac4dfca
Set up Emotion ThemeProvider + types
cee-chen Jul 6, 2023
64b1226
Add unit tests
cee-chen Jul 11, 2023
c8b6787
Add documentation warning against internal usage
cee-chen Jul 11, 2023
0107f93
Add consumer-facing documentation
cee-chen Jul 12, 2023
e27ee4c
changelog
cee-chen Jul 12, 2023
ca6a49d
[PR feedback] Copy
cee-chen Jul 12, 2023
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
49 changes: 49 additions & 0 deletions src-docs/src/views/theme/consuming_emotion_theme.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React from 'react';
import { ThemeProvider, css } from '@emotion/react';
import { EuiIcon, EuiText } from '../../../../src';

export default () => {
return (
<EuiText>
<p
css={({ euiTheme }) => css`
1Copenut marked this conversation as resolved.
Show resolved Hide resolved
background-color: ${euiTheme.colors.lightestShade};
padding: ${euiTheme.size.l};
`}
>
<EuiIcon
type="faceHappy"
// The current `colorMode` is also available in the passed Emotion theme
// which may be useful for certain conditional styles
css={({ euiTheme, colorMode }) => ({
color:
colorMode === 'LIGHT'
? euiTheme.colors.primary
: euiTheme.colors.accent,
})}
/>{' '}
This box sets its icon color, background color, and padding via Emotion
theme context
</p>

<ThemeProvider
theme={{
// @ts-ignore - if providing your own Emotion theme, create your own emotion.d.ts - see https://emotion.sh/docs/typescript#define-a-theme
brandColor: 'pink',
backgroundColor: 'black',
padding: '1rem',
}}
>
<p
css={(theme: any) => css`
color: ${theme.brandColor};
background-color: ${theme.backgroundColor};
padding: ${theme.padding};
`}
>
This box sets its own Emotion ThemeProvider and theme variables
</p>
</ThemeProvider>
</EuiText>
);
};
33 changes: 33 additions & 0 deletions src-docs/src/views/theme/theme_example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ const consumingSource = require('!!raw-loader!./consuming');
import { ConsumingHOC } from './consuming_hoc';
const consumingHOCSource = require('!!raw-loader!./consuming_hoc');

import ConsumingEmotionTheme from './consuming_emotion_theme';
const consumingEmotionThemeSource = require('!!raw-loader!./consuming_emotion_theme');

import OverrideSimple from './override_simple';
const overrideSimpleSource = require('!!raw-loader!./override_simple');

Expand Down Expand Up @@ -154,6 +157,36 @@ export const ThemeExample = {
),
demo: <ConsumingHOC />,
},
{
title: "Consuming with Emotion's theming",
source: [
{
type: GuideSectionTypes.TSX,
code: consumingEmotionThemeSource,
},
],
text: (
<>
<p>
<strong>EuiThemeProvider</strong> by default sets an{' '}
<EuiLink href="https://emotion.sh/docs/theming" target="_blank">
Emotion theme context
</EuiLink>{' '}
with the results of <strong>useEuiTheme()</strong>. This is a
syntactical sugar convenience that allows you to take advantage of
Emotion's <EuiCode>styled</EuiCode> syntax, or use a function in the{' '}
<EuiCode>css</EuiCode> prop.
</p>
<p>
If you prefer to use or access your own custom Emotion theme, you
can completely override EUI's passed theme at any time with your own{' '}
<EuiCode>ThemeProvider</EuiCode> - see the second box below for an
example.
</p>
</>
),
demo: <ConsumingEmotionTheme />,
},
{
title: 'Simple instance overrides',
source: [
Expand Down
17 changes: 17 additions & 0 deletions src/custom_typings/emotion.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* 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 '@emotion/react';
import { UseEuiTheme } from '../services/theme';

/**
* @see https://emotion.sh/docs/typescript#define-a-theme
*/
declare module '@emotion/react' {
export interface Theme extends UseEuiTheme {}
}
84 changes: 84 additions & 0 deletions src/services/theme/emotion.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* 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 from 'react';
import { ThemeProvider } from '@emotion/react';
import { render } from '../../test/rtl';

import { EuiTextColor } from '../../components/text';
import { EuiEmotionThemeProvider } from './emotion';

describe('EuiEmotionThemeProvider', () => {
it("allows consumers to use Emotion's theme context by default", () => {
const { container, getByTestSubject } = render(
<EuiEmotionThemeProvider>
<div
css={({ euiTheme }) => ({ color: euiTheme.colors.primary })}
data-test-subj="consumer"
>
hello world
</div>
</EuiEmotionThemeProvider>
);

expect(getByTestSubject('consumer')).toHaveStyleRule('color', '#07C');

expect(container.firstChild).toMatchInlineSnapshot(`
<div
class="css-cs4x42"
data-test-subj="consumer"
>
hello world
</div>
`);
});

it("allows consumers to override EUI's ThemeProvider with their own theme", () => {
const customTheme = {
brandColor: 'pink',
};

const { container, getByTestSubject } = render(
<EuiEmotionThemeProvider>
{/* @ts-ignore - consumers would set their own emotion.d.ts */}
<ThemeProvider theme={customTheme}>
<div
css={(theme: any) => ({ color: theme.brandColor })}
data-test-subj="consumer"
>
hello
</div>
{/* Custom Emotion themes should not break EUI's own Emotion styles */}
<EuiTextColor color="accent" data-test-subj="eui">
world
</EuiTextColor>
</ThemeProvider>
</EuiEmotionThemeProvider>
);

expect(getByTestSubject('consumer')).toHaveStyleRule('color', 'pink');
expect(getByTestSubject('eui')).toHaveStyleRule('color', '#ba3d76');

expect(container).toMatchInlineSnapshot(`
<div>
<div
class="css-18ry2co"
data-test-subj="consumer"
>
hello
</div>
<span
class="emotion-euiTextColor-accent"
data-test-subj="eui"
>
world
</span>
</div>
`);
});
});
28 changes: 28 additions & 0 deletions src/services/theme/emotion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* 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 { ThemeProvider } from '@emotion/react';

import { useEuiTheme } from './hooks';

/**
* @see https://emotion.sh/docs/theming
* This Emotion theme provider is added for *consumer usage* & convenience only.
*
* EUI should stick to using our own context/`useEuiTheme` internally
* instead of Emotion's shorthand `css={theme => {}}` API. If consumers
* set their own theme via <ThemeProvider>; EUI's styles should continue
* working as-is.
*/
export const EuiEmotionThemeProvider: FunctionComponent<
PropsWithChildren<{}>
> = ({ children }) => {
const euiThemeContext = useEuiTheme();
return <ThemeProvider theme={euiThemeContext}>{children}</ThemeProvider>;
Comment on lines +26 to +27
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 also pretty 50/50 on providing the entire theme context (i.e. colorMode and modify alongside with the euiTheme vars) or just euiTheme itself, i.e.

Suggested change
const euiThemeContext = useEuiTheme();
return <ThemeProvider theme={euiThemeContext}>{children}</ThemeProvider>;
const { euiTheme } = useEuiTheme();
return <ThemeProvider theme={euiTheme}>{children}</ThemeProvider>;

To be honest, I would assume most consumers won't need color mode info, but I think it might be syntactically confusing to return a theme prop that doesn't match the return structure of our own useEuiTheme() hook. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd plus one on having the return structure match our useEuiTheme() hook. It might be verbose, but I'm a big fan of consistency.

};
2 changes: 1 addition & 1 deletion src/services/theme/hooks.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import React from 'react';
import { renderHook } from '@testing-library/react-hooks';
import { render } from '@testing-library/react';

import { setEuiDevProviderWarning } from './provider';
import { setEuiDevProviderWarning } from './warning';
import {
useEuiTheme,
UseEuiTheme,
Expand Down
2 changes: 1 addition & 1 deletion src/services/theme/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
EuiColorModeContext,
defaultComputedTheme,
} from './context';
import { getEuiDevProviderWarning } from './provider';
import { getEuiDevProviderWarning } from './warning';
import {
EuiThemeColorModeStandard,
EuiThemeModifications,
Expand Down
7 changes: 2 additions & 5 deletions src/services/theme/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,8 @@ export {
export type { UseEuiTheme, WithEuiThemeProps } from './hooks';
export { useEuiTheme, withEuiTheme, RenderWithEuiTheme } from './hooks';
export type { EuiThemeProviderProps } from './provider';
export {
EuiThemeProvider,
getEuiDevProviderWarning,
setEuiDevProviderWarning,
} from './provider';
export { EuiThemeProvider } from './provider';
export { getEuiDevProviderWarning, setEuiDevProviderWarning } from './warning';
export {
buildTheme,
computed,
Expand Down
11 changes: 4 additions & 7 deletions src/services/theme/provider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
EuiModificationsContext,
EuiColorModeContext,
} from './context';
import { EuiEmotionThemeProvider } from './emotion';
import { buildTheme, getColorMode, getComputed, mergeDeep } from './utils';
import {
EuiThemeColorMode,
Expand All @@ -36,12 +37,6 @@ import {
EuiThemeModifications,
} from './types';

type LEVELS = 'log' | 'warn' | 'error';
let providerWarning: LEVELS | undefined = undefined;
export const setEuiDevProviderWarning = (level: LEVELS | undefined) =>
(providerWarning = level);
export const getEuiDevProviderWarning = () => providerWarning;

export interface EuiThemeProviderProps<T> {
theme?: EuiThemeSystem<T>;
colorMode?: EuiThemeColorMode;
Expand Down Expand Up @@ -190,7 +185,9 @@ export const EuiThemeProvider = <T extends {} = {}>({
<EuiModificationsContext.Provider value={modifications}>
<EuiThemeContext.Provider value={theme}>
<EuiNestedThemeContext.Provider value={nestedThemeContext}>
{renderedChildren}
<EuiEmotionThemeProvider>
{renderedChildren}
</EuiEmotionThemeProvider>
Comment on lines +188 to +190
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 really 50/50 on baking this into EuiThemeProvider by default.

The other option would be to simply export <EuiEmotionThemeProvider> and allow consumers to opt into it, e.g.

<EuiThemeProvider>
  <EuiEmotionThemeProvider>
    // component that can use `css={theme => {}}` syntax
  </EuiEmotionThemeProvider>
</EuiThemeProvider>

The major downside to that is that consumers will need to remember to re-include EuiEmotionThemeProvider every time they nest EuiThemeProvider for component-level theming, i.e.

// Top level of app
<EuiProvider>
  <EuiEmotionThemeProvider>
    // the `theme` prop vars will be correct here

    <EuiThemeProvider colorMode="inverse">
      // the `theme` prop vars will NOT be correct here

      <EuiEmotionThemeProvider>
        // the `theme` prop vars WILL be correct here
      </EuiEmotionThemeProvider>
    </EuiThemeProvider>
  </EuiEmotionThemeProvider>
</EuiProvider>

The flip side is that baking in Emotion's theme context into EuiThemeProvider would be more inflexible for consumers who don't want our theme or want to use/set their own Emotion theme. Do we want to support those consumers though? see #5351

Copy link
Contributor

Choose a reason for hiding this comment

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

By baking in the entire thing, do we provide enough of an escape hatch for folks with the modify object? In my mind, EUI powers Kibana as its first order of business, so we should be tailoring our approach to that first and allow folks to mod if that's what they need to do.

To me it's like CRA. There's a clear set of defaults, but if folks absolutely need to mod things, they can eject and tailor the experience. The analogy isn't 1:1, but I look at it similarly in DX terms.

Copy link
Member Author

@cee-chen cee-chen Jul 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

By baking in the entire thing, do we provide enough of an escape hatch for folks with the modify object?

IMO, I think it's enough of an escape hatch. The other option we can consider is adding an includeEmotionTheme prop, e.g.

<EuiThemeProvider includeEmotionTheme={false}>

I think with both of those options, we should have enough of an escape hatch for consumers who want to set their own Emotion theme/styling and not use EUI's.

edit: Ah, I just saw your comment down below where you don't think that prop is necessary. 🤔 I'd be good not including it for and waiting to see what feedback we get from consumers first!

</EuiNestedThemeContext.Provider>
</EuiThemeContext.Provider>
</EuiModificationsContext.Provider>
Expand Down
16 changes: 16 additions & 0 deletions src/services/theme/warning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/*
* 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.
*/

type LEVELS = 'log' | 'warn' | 'error';

let providerWarning: LEVELS | undefined = undefined;

export const setEuiDevProviderWarning = (level: LEVELS | undefined) =>
(providerWarning = level);

export const getEuiDevProviderWarning = () => providerWarning;
2 changes: 2 additions & 0 deletions upcoming_changelogs/6913.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Updated `EuiThemeProvider` to set an Emotion theme context that returns the values of `useEuiTheme()`

8 changes: 8 additions & 0 deletions wiki/contributing-to-eui/developing/writing-styles-with-emotion.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,3 +545,11 @@ Emotion provides its own `createElement` function; existing uses of `import {cre
Unfortunately, a limitation of the CSS-in-JS syntax parser we're using is that `//` comments throw this error (see https://github.com/hudochenkov/postcss-styled-syntax#known-issues).

You must convert all `//` comments to standard CSS `/* */` comments instead.

### Should I use Emotion's `css={theme => {}}` API?

No. The [Emotion theme context](https://emotion.sh/docs/theming) that we include by default in `EuiThemeProvider` is intended for **consumer usage** and convenience, particularly with the goal of making adoption by Kibana devs easier.

It is not intended for internal EUI usage, primarily because it can be too easily overridden by consumers who want to use their own custom Emotion theme vars and set their own `<ThemeProvider>`. If this happens, and we're relying on Emotion's theme context, all of EUI's styles will break.

When you're styling EUI components internally, you should use only EUI's theme context/`useEuiTheme()`, and not on Emotion's theme context (i.e., do not use the `css={theme => {}}` API).
Loading