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.

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

[regression] Handle deprecated KibanaThemeProvider uses to include KibanaRenderContextProvider #163103

Merged
merged 7 commits into from
Aug 4, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Aug 3, 2023

Summary

Unfortunately, #161914 regressed #162365 in that many plugins and their Emotion styles (including EUI emotion styles) are now missing a cache and are being appended to to the end of the document <head> as opposed to within <meta name="emotion">.

What appears to be happening is many plugins are using a parent <KibanaThemeProvider> but not a parent <KibanaRootContextProvider> (not sure if this work is TBD or in progress). This means that a parent <EuiProvider>, (which determines the cache insertion of child Emotion styled components) is missing, which is causing several CSS specificity bugs, e.g. around datagrid.

As a somewhat-bandaid-y fix, I've bogarted EUI's nested provider context to check if the theme provider has a parent EuiProvider, and if it doesn't, to use KibanaEuiProvider instead of KibanaThemeProvider. This should set up the caches and context if needed, or otherwise simply use the original KibanaThemeProvider component.

FYI

If you see an error like this, in console dev tools, that means an Emotion cache somewhere is improperly set up. All of EUI's and Kibana's default caches should have .compat = true set, which silences these errors.

Screenshot 2023-08-03 at 8 40 06 AM

QA

Previous/broken behavior

  • In your browser devtools inspector, open the document <head>
  • Under <meta name="emotion">, you'll only see a couple hundred style nodes:

  • If you scroll to the bottom of <head>, you should see a lot of <style data-emotion="css"> nodes:

Fixed behavior in this PR

  • In your browser devtools inspector, open the document <head>
  • Under <meta name="emotion">, you should now see well over thousands of styles

  • You should no longer see many <style data-emotion="css"> with EUI styles at the end of the head (there's a lot of <script> tags instead). There's still a few, but they appear to not be EUI styles (e.g. it's a plugin's random Emotion styles that they're using without a cache set up).

- this removes the need for a manual CurrentEuiBreakpointProvider and fixes issues with plugins' Emotions styles going to the wrong cache
- not sure about the copy on this 🤷
@cee-chen cee-chen marked this pull request as ready for review August 3, 2023 19:30
@cee-chen cee-chen requested review from a team as code owners August 3, 2023 19:30
@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes v8.10.0 EUI labels Aug 3, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

updated snapshots look fine
LGTM

@clintandrewhall
Copy link
Contributor

I think we can combine these changes with some of mine from #163137. I think I prefer to detect and handle it internally as you do here... it certainly makes it easier than making the i18n contract optional.

Let's chat tomorrow.

Copy link
Contributor

@clintandrewhall clintandrewhall left a comment

Choose a reason for hiding this comment

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

Thanks for helping with this, @cee-chen !

@cee-chen
Copy link
Contributor Author

cee-chen commented Aug 4, 2023

Thanks for fixing CI for me!! 😁

@cee-chen cee-chen enabled auto-merge (squash) August 4, 2023 15:34
@cee-chen cee-chen disabled auto-merge August 4, 2023 15:35
@clintandrewhall clintandrewhall changed the title Check for KibanaThemeProviders missing a parent KibanaEuiProvider [regression] Handle deprecated KibanaThemeProvider uses to include KibanaRenderContextProvider Aug 4, 2023
@cee-chen cee-chen enabled auto-merge (squash) August 4, 2023 15:39
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
core 471 474 +3
interactiveSetup 53 62 +9
kibanaReact 316 325 +9
kibanaUtils 172 181 +9
total +30

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/react-kibana-context-root 1 4 +3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
kibanaReact 209.3KB 209.3KB +12.0B
kibanaUtils 60.8KB 60.8KB +4.0B
total +16.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/react-kibana-context-root 1 0 -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 369.9KB 372.0KB +2.1KB
interactiveSetup 58.5KB 61.3KB +2.9KB
kibanaReact 51.3KB 54.3KB +3.0KB
kibanaUtils 70.7KB 73.6KB +2.9KB
total +10.8KB
Unknown metric groups

API count

id before after diff
@kbn/react-kibana-context-render 4 3 -1
@kbn/react-kibana-context-root 4 9 +5
total +4

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@cee-chen cee-chen merged commit f2b07fc into elastic:main Aug 4, 2023
@cee-chen cee-chen deleted the cache-50-bandaid branch August 4, 2023 16:11
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 4, 2023
jughosta added a commit that referenced this pull request Aug 11, 2023
…163387)

- Closes #163332

## Summary

This PR fixes dark mode issues which surfaced after
#161914 and
#163103

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI release_note:skip Skip the PR/issue when compiling release notes v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants