[Emotion] Nested `EuiThemeProvider`s now render a wrapper setting the correct inherited text `color` #6775

cee-chen · 2023-05-15T23:29:09Z

Summary

I strongly recommend following along by commit - the existing issue is already fairly complex, and there's some additional cleanup happening.

Non-portalled content

Nested content would previously require an extra nested <EuiText color="default"> in order for text color to inherit properly:

Now, an extra configurable <span className="euiThemeProvider> wrapper is added automatically to nested EuiThemeProviders, removing the need for the extra manual EuiText:

Consumers can optionally pass <EuiThemeProvider wrapperProps={{ cloneElement: true }}> to clone the necessary CSS class onto a single passed child, if they wish to avoid the wrapper for layout or other reasons:

ℹ️ Note that the top level EuiProvider will never render a wrapper - only nested EuiThemeChildren.

Portalled content

Portalled content needs to be handled separately, as it will not inherit from the added wrapper, and will instead almost certainly be positioned to inherit directly from <body>. As a result, I added some extra logic in 3c71c2a that sets the color CSS directly on the data-euiportal div wrapper. In an attempt to reduce downstream snapshot churn, I set up portals to only add this CSS if their inherited color is different from the global body color.

ℹ️ Note: I deliberately avoided using this type of conditional logic for the un-portalled <EuiThemeProvider> spans - the unpredictability of an entire DOM wrapper conditionally appearing and disappearing has far more consequences on layout breakage than just a className. In contrast, the data-euiportal div is always present for portals.

QA

Go to https://eui.elastic.co/pr_6775/#/theming/color-mode#rendering-a-specific-color-mode
Confirm that text in the panels render as the correct colors
Confirm that popovers, modals, and flyouts opened from the nested theme providers render as the correct colors
On the main page bar, switch the docs color mode to the opposite of what you had it as (likely dark)
Confirm that the inverse panels correctly updated, and the first two light/dark mode panels did not
Confirm that the inverse popovers, modals, and flyouts updated correctly

General checklist

Checked in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs (using @default if default values are missing) and playground toggles
Added documentation
Added or updated jest and cypress tests
A changelog entry exists and is marked appropriately
Checked for breaking changes and labeled appropriately

~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~- [ ] Updated the Figma library counterpart~~

context sets up: - whether the current theme should render a `span` (i.e., is global theme or is nested) - sets up a resable `className` string that can be reused across both React elements and DOM nodes (important for portals)

+ document new behavior

- for consumers who have a single child / where an extra rendered wrapper will cause layout issues + update `EuiButton`s with `color="ghost"` to use cloneElement

- it's causing a snapshot diff when it shouldn't/doesn't need to; in general we should no longer be taking mounted snapshots

+ add a 3rd level nested demo w/ `cloneElement`

- there's already a `data-euiportal` wrapper around all portals to bogart, so no need to create one - in an attempt to reduce snapshot churn in Kibana's side of things, I've opted to only render a CSS class if the theme color is different from the `body` color + [tech debt] switch `portal.test.tsx` tests to RTL render

- e.g., EuiModal, EuiFlyout + [tech debt] Add missing overlay mask tests using RTL

- should use `cloneElement` behavior instead of a wrapper - no longer needs to set `color` + [tech debt] update tests to use RTL

- appears to have been replaced by `color_mode/inverse` instead

kibanamachine · 2023-05-16T00:18:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_6775/

cee-chen · 2023-05-16T00:41:30Z

Hey @elastic/eui-team! Does anyone on here have a spare half hour to an hour to review this PR this week? Following the QA steps checklist and performing a quick code review (that I strongly recommend following by commit for) should hopefully be all that's needed. This PR is not urgent and does not need to get in before Tuesday's release.

Once this merges and hits Kibana, we'll almost certainly need to audit all usages of <EuiThemeProvider> in Kibana to ensure no layout issues occur as a result of this change. I can assist with whoever is is handling the Kibana upgrade for that week; please ping me for that.

I'll also be making use of Matthias's kibana a-la-carte w/ this branch ahead of time to see what fails and passes in Kibana CI ahead of time. Fingers crossed!

breehall

This is great Cee! It's an elegant solution to a complex problem. I followed commit-by-commit and also used your testing steps. Here are a few of my overall thoughts:

This was great walkthrough on some of our theming. Sometimes, our theming feels complex to me and this was a great exercise.
I really like the new documentation and examples that were added! I can see this being more helpful to consumers and reducing the frequency that we see this question (and variants) pop up in the EUI channel. I can also see this being very helpful for our team for visually testing.
I'm very happy this addresses the EuiBottomBar theming issue. We've also seen that popup quite a few times as well.
The consideration for layout fluctuation and the addition of cloneElement in wrapperProps is handy!

breehall · 2023-05-17T02:03:30Z

src/components/portal/portal.tsx

@@ -41,6 +43,8 @@ export interface EuiPortalProps {
 }

 export class EuiPortal extends Component<EuiPortalProps> {
+  static contextType = EuiNestedThemeContext;


I haven't really seen static since my Java days. This was a good refresher.

cee-chen · 2023-05-17T16:14:08Z

Thanks for the thorough review Bree!

## Summary `@elastic/eui@80.0.0` ⏩ `@elastic/eui@81.0.0` --- ## [`81.0.0`](https://github.com/elastic/eui/tree/v81.0.0) - Added ability to set `options.checked` to "mixed" in `EuiSelectable` ([#6774](elastic/eui#6774)) **Bug fixes** - Portalled components (e.g. `EuiPopover`, `EuiModal`, `EuiFlyout`) will correctly inherit text color from its nearest `EuiThemeProvider` parent. `<EuiText color="default">` is no longer needed. ([#6775](elastic/eui#6775)) **Breaking changes** - `EuiSelectable` no longer renders a `data-test-selected` attribute on its list items. Use the `aria-checked` property instead ([#6774](elastic/eui#6774)) - Nested `EuiThemeProvider`s now render a wrapping `<span>` element in order to correctly set the inherited text `color` of all descendants. `<EuiText color="default">` is no longer needed. ([#6775](elastic/eui#6775)) --------- Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen@elastic.co>

cee-chen added 7 commits May 15, 2023 13:03

Update docs to remove now-unnecessary color CSS

2f697af

+ document new behavior

Add wrapperProps customization for new span wrapper

00386a3

Add cloneElement support

fc16c8e

- for consumers who have a single child / where an extra rendered wrapper will cause layout issues + update `EuiButton`s with `color="ghost"` to use cloneElement

[misc] fix incorrect color="ghost" usage in docs skip link

2f2ad58

[misc cleanup] remove mounted snapshot

15ecc14

- it's causing a snapshot diff when it shouldn't/doesn't need to; in general we should no longer be taking mounted snapshots

[docs] Update colorMode demo to include portals for testing

43774c3

+ add a 3rd level nested demo w/ `cloneElement`

cee-chen added the breaking change label May 15, 2023

cee-chen added 5 commits May 15, 2023 16:54

Fix portals with overlay masks overriding theme color

10fd747

- e.g., EuiModal, EuiFlyout + [tech debt] Add missing overlay mask tests using RTL

Update EuiBottomBar

19f6074

- should use `cloneElement` behavior instead of a wrapper - no longer needs to set `color` + [tech debt] update tests to use RTL

[docs][cleanup] Remove unused inverse src-docs file

9924f1e

- appears to have been replaced by `color_mode/inverse` instead

changelog

c7ce7ef

cee-chen force-pushed the emotion/nested-color-modes branch from 75e5835 to c7ce7ef Compare May 15, 2023 23:55

cee-chen added the emotion label May 16, 2023

cee-chen marked this pull request as ready for review May 16, 2023 00:29

breehall approved these changes May 17, 2023

View reviewed changes

cee-chen merged commit f516c12 into elastic:main May 17, 2023

cee-chen deleted the emotion/nested-color-modes branch May 17, 2023 16:14

1Copenut mentioned this pull request May 23, 2023

Upgrade EUI to v81.0.0 elastic/kibana#158330

Merged

cee-chen mentioned this pull request Oct 23, 2023

[EuiProvider] Provide data about parent and default theme configurations #6210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Nested `EuiThemeProvider`s now render a wrapper setting the correct inherited text `color` #6775

[Emotion] Nested `EuiThemeProvider`s now render a wrapper setting the correct inherited text `color` #6775

cee-chen commented May 15, 2023 •

edited

Loading

kibanamachine commented May 16, 2023

cee-chen commented May 16, 2023 •

edited

Loading

breehall left a comment

breehall May 17, 2023

cee-chen commented May 17, 2023

[Emotion] Nested EuiThemeProviders now render a wrapper setting the correct inherited text color #6775

[Emotion] Nested EuiThemeProviders now render a wrapper setting the correct inherited text color #6775

Conversation

cee-chen commented May 15, 2023 • edited Loading

Summary

Non-portalled content

Portalled content

QA

General checklist

kibanamachine commented May 16, 2023

cee-chen commented May 16, 2023 • edited Loading

breehall left a comment

Choose a reason for hiding this comment

breehall May 17, 2023

Choose a reason for hiding this comment

cee-chen commented May 17, 2023

[Emotion] Nested `EuiThemeProvider`s now render a wrapper setting the correct inherited text `color` #6775

[Emotion] Nested `EuiThemeProvider`s now render a wrapper setting the correct inherited text `color` #6775

cee-chen commented May 15, 2023 •

edited

Loading

cee-chen commented May 16, 2023 •

edited

Loading