[EuiProvider] Add a check and dev warning+early return for nested usage #6949

cee-chen · 2023-07-14T00:14:28Z

Summary

@clintandrewhall is working on ensuring all Kibana plugins have EuiProvider instantiated (in order for color mode to work correctly across Kibana). Unfortunately, due to the complexity of Kibana and its multiple teams, there are several instances of teams using or nesting EuiProvider when they really want EuiThemeProvider instead.

Things that occur with multiple nested EuiProvider's that we want to avoid:

Global reset and utilities CSS styles are re-inserted/duplicated into the head
An extra <span> wrapper is rendered (due to the nested EuiThemeProvder)
Extra window resize listeners are attached when they don't need to be (breakpoints provider)
The componentDefaults context will be overridden, potentially unintentionally

In light of these side effects, and to help facilitate Shared UX's work, we're 1. baking into EuiProvider a lack of ability to nest multiple usages, and 2. warning if the above happens, and 3. clarifying our docs to hopefully make the relationship between <EuiProvider> and <EuiThemeProvider> clearer.

QA

General checklist

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

- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
~~- [ ] Updated the Figma library counterpart~~

- they don't really belong in `EuiProvider`

- make `EuiProvider`s tests more basic in comparison - use `toHaveStyleRule` instead of snapshots

…ge is detected

- clarify in as many places as possible - remove uncertain language (e.g. 'currently', 'future') that were still not yet figured out when docs were written - try to remove overlap in docs between provider and theme provider, try to clarify nested usage of theme provider - add examples to warning section

cee-chen · 2023-07-14T00:16:43Z

@clintandrewhall feel free to skip reviewing the commits that are labelled [cleanup] - I was trying to get rid of our cumbersome Enzyme snapshots and switch entirely to RTL. The 365e6e3 commit is probably of most interest to you in terms of promised functionality (warning + returning early without instantiating any further EuiProvider logic).

cee-chen · 2023-07-14T00:23:58Z

@clintandrewhall I'd also love feedback on the updated docs for EuiProvider and EuiThemeProvider (once staging is done spinning up). I totally agree the language was really not clear around their usage and it's hopefully much better now. Please feel free to leave any suggestions if you think any of the copy there could be improved!

kibanamachine · 2023-07-14T00:43:53Z

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

kibanamachine · 2023-07-15T02:02:48Z

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

clintandrewhall

This looks great, thank you so much. I'll try to test this against Kibana on Monday.

clintandrewhall · 2023-07-15T17:32:22Z

src/components/provider/provider.tsx


 const isEmotionCacheObject = (
  obj: EmotionCache | Object
 ): obj is EmotionCache => obj.hasOwnProperty('key');

 export interface EuiProviderProps<T>
-  extends Omit<EuiThemeProviderProps<T>, 'children' | 'theme'>,
+  extends Pick<EuiThemeProviderProps<T>, 'colorMode' | 'modify'>,


clintandrewhall · 2023-07-15T17:33:31Z

src/services/theme/warning.ts

@@ -14,3 +14,20 @@ export const setEuiDevProviderWarning = (level: LEVELS | undefined) =>
  (providerWarning = level);

 export const getEuiDevProviderWarning = () => providerWarning;
+
+// Not a public top-level EUI export, currently for internal use
+export const emitEuiProviderWarning = (providerMessage: string) => {


clintandrewhall · 2023-07-15T17:34:34Z

Do you want anyone from @elastic/eui-team to review this before committing? It looks great to me, and I can test it in Kibana on Monday.

kibanamachine · 2023-07-15T17:59:26Z

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

src/services/breakpoint/is_within_hooks.spec.tsx

kibanamachine · 2023-07-17T17:08:17Z

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

tkajtoch

The changes look great! Feel free to merge as soon as CI passes :)

kibanamachine · 2023-07-17T18:01:03Z

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

## Summary `eui@84.0.0` ⏩ `eui@85.0.1` ## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1) **Bug fixes** - Fixed `EuiFilterGroup`'s responsive styles ([#6983](elastic/eui#6983)) ## [`85.0.0`](https://github.com/elastic/eui/tree/v85.0.0) - Updated `EuiThemeProvider` to set an Emotion theme context that returns the values of `useEuiTheme()` ([#6913](elastic/eui#6913)) - Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous size of `m` ([#6928](elastic/eui#6928)) - Added new `s` sizing to `EuiStepsHorizontal` ([#6928](elastic/eui#6928)) - Added `at` and `key` icon glyphs. ([#6934](elastic/eui#6934)) - Added a new `cloneElementWithCss` Emotion utility ([#6939](elastic/eui#6939)) - Updated `EuiPopover` to allow consumer control of all `focusTrapProps` ([#6955](elastic/eui#6955)) **Bug fixes** - Fixed `EuiDataGrid` height calculation bug when browser zoom levels are not 100% ([#6895](elastic/eui#6895)) - Fixed `EuiTab` not correctly passing selection color state to `prepend` and `append` children ([#6938](elastic/eui#6938)) - Fixed `EuiInputPopover` to allow consumer control of its focus trap via `focusTrapProps` ([#6955](elastic/eui#6955)) **Breaking changes** - `EuiProvider` will no longer render multiple or duplicate nested instances of itself. If a nested `EuiProvider` is detected, that instance will return early without further processing, and will warn if configured to do so via `setEuiDevProviderWarning`. For nested theming, use `EuiThemeProvider` instead. ([#6949](elastic/eui#6949)) - Removed `onTrapDeactivation` prop from `EuiPopover`. Use `focusTrapProps.onDeactivation` instead ([#6955](elastic/eui#6955)) **CSS-in-JS conversions** - Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed styles attached to `.euiFilterGroup__popoverPanel` ([#6957](elastic/eui#6957)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

## Summary `eui@84.0.0` ⏩ `eui@85.0.1` ## [`85.0.1`](https://github.com/elastic/eui/tree/v85.0.1) **Bug fixes** - Fixed `EuiFilterGroup`'s responsive styles ([elastic#6983](elastic/eui#6983)) ## [`85.0.0`](https://github.com/elastic/eui/tree/v85.0.0) - Updated `EuiThemeProvider` to set an Emotion theme context that returns the values of `useEuiTheme()` ([elastic#6913](elastic/eui#6913)) - Added `size` prop to `EuiStepsHorizontal`, defaulting to the previous size of `m` ([elastic#6928](elastic/eui#6928)) - Added new `s` sizing to `EuiStepsHorizontal` ([elastic#6928](elastic/eui#6928)) - Added `at` and `key` icon glyphs. ([elastic#6934](elastic/eui#6934)) - Added a new `cloneElementWithCss` Emotion utility ([elastic#6939](elastic/eui#6939)) - Updated `EuiPopover` to allow consumer control of all `focusTrapProps` ([elastic#6955](elastic/eui#6955)) **Bug fixes** - Fixed `EuiDataGrid` height calculation bug when browser zoom levels are not 100% ([elastic#6895](elastic/eui#6895)) - Fixed `EuiTab` not correctly passing selection color state to `prepend` and `append` children ([elastic#6938](elastic/eui#6938)) - Fixed `EuiInputPopover` to allow consumer control of its focus trap via `focusTrapProps` ([elastic#6955](elastic/eui#6955)) **Breaking changes** - `EuiProvider` will no longer render multiple or duplicate nested instances of itself. If a nested `EuiProvider` is detected, that instance will return early without further processing, and will warn if configured to do so via `setEuiDevProviderWarning`. For nested theming, use `EuiThemeProvider` instead. ([elastic#6949](elastic/eui#6949)) - Removed `onTrapDeactivation` prop from `EuiPopover`. Use `focusTrapProps.onDeactivation` instead ([elastic#6955](elastic/eui#6955)) **CSS-in-JS conversions** - Converted `EuiFilterGroup` and `EuiFilterButton` to Emotion; Removed styles attached to `.euiFilterGroup__popoverPanel` ([elastic#6957](elastic/eui#6957)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen added 9 commits July 13, 2023 13:28

[setup] DRY out warning logic and write more robust unit tests for it

a8df8cc

[cleanup] Move EuiThemeProvider nested tests to theme provider file

89e61de

- they don't really belong in `EuiProvider`

[cleanup] Improve EuiThemeProvider's unit testing

f2d0903

- make `EuiProvider`s tests more basic in comparison - use `toHaveStyleRule` instead of snapshots

[cleanup] Convert more EuiProvider tests to RTL from Enzyme

fe02eb1

[cleanup] Convert EuiProvider cache tests to RTL from Enzyme

13a4bf3

Set up a context to check for nested EuiProviders

73bf66c

Update EuiProvider to return early and emit a warning if nested usa…

365e6e3

…ge is detected

Fix types on EuiProvider's props table

61e8630

cee-chen added the breaking change label Jul 14, 2023

cee-chen requested a review from clintandrewhall July 14, 2023 00:14

changelog

16f08a4

cee-chen marked this pull request as ready for review July 14, 2023 00:24

Fix Cypress tests failing due to nested EuiProviders

1db5f22

clintandrewhall approved these changes Jul 15, 2023

View reviewed changes

tkajtoch reviewed Jul 17, 2023

View reviewed changes

src/services/breakpoint/is_within_hooks.spec.tsx Outdated Show resolved Hide resolved

cee-chen force-pushed the provider/nested branch from c849b9b to e03d477 Compare July 17, 2023 17:33

[PR feedback] Allow cy.mount to have a configurable provider props

0fadabd

cee-chen force-pushed the provider/nested branch from e03d477 to 0fadabd Compare July 17, 2023 17:35

tkajtoch approved these changes Jul 17, 2023

View reviewed changes

cee-chen enabled auto-merge (squash) July 17, 2023 17:48

cee-chen merged commit 42ecae5 into elastic:main Jul 17, 2023

cee-chen deleted the provider/nested branch July 17, 2023 18:15

tkajtoch mentioned this pull request Jul 19, 2023

Upgrade @elastic/eui to 85.0.1 elastic/kibana#162209

Merged

cee-chen mentioned this pull request Oct 23, 2023

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

Closed

cee-chen mentioned this pull request Jun 11, 2024

[Docs] EuiProvider custom breakpoints example no longer works #7829

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EuiProvider] Add a check and dev warning+early return for nested usage #6949

[EuiProvider] Add a check and dev warning+early return for nested usage #6949

cee-chen commented Jul 14, 2023 •

edited

Loading

cee-chen commented Jul 14, 2023

cee-chen commented Jul 14, 2023 •

edited

Loading

kibanamachine commented Jul 14, 2023

kibanamachine commented Jul 15, 2023

clintandrewhall left a comment

clintandrewhall Jul 15, 2023

clintandrewhall Jul 15, 2023

clintandrewhall commented Jul 15, 2023

kibanamachine commented Jul 15, 2023

kibanamachine commented Jul 17, 2023

tkajtoch left a comment

kibanamachine commented Jul 17, 2023

[EuiProvider] Add a check and dev warning+early return for nested usage #6949

[EuiProvider] Add a check and dev warning+early return for nested usage #6949

Conversation

cee-chen commented Jul 14, 2023 • edited Loading

Summary

QA

General checklist

cee-chen commented Jul 14, 2023

cee-chen commented Jul 14, 2023 • edited Loading

kibanamachine commented Jul 14, 2023

kibanamachine commented Jul 15, 2023

clintandrewhall left a comment

Choose a reason for hiding this comment

clintandrewhall Jul 15, 2023

Choose a reason for hiding this comment

clintandrewhall Jul 15, 2023

Choose a reason for hiding this comment

clintandrewhall commented Jul 15, 2023

kibanamachine commented Jul 15, 2023

kibanamachine commented Jul 17, 2023

tkajtoch left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 17, 2023

cee-chen commented Jul 14, 2023 •

edited

Loading

cee-chen commented Jul 14, 2023 •

edited

Loading