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

[EuiProviders] Warn Developer if EuiProvider is missing #184608

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 31, 2024

Summary

Addresses https://github.com/elastic/kibana-team/issues/805

Rollout plan

These steps will graduate toward a consistency of Dark Mode theming (and I18n, and Analytics for Error Boundaries) by ensuring that EUI components are always rendered with KibanaRenderContextProvider wrapping the root of the rendering tree.

The final behavior: in Dev mode when KibanaRenderContextProvider is missing, developers will see a console.error and a toast Error message. In CI environments, the error scenario will cause the EUI components to fail with an error and developers will have to address the issue with a fix by viewing the test logs.

  1. [Done] Initial error behavior: throw error if EUI Provider is missing in a Draft PR. In CI of that Draft PR, EUI components will fail to render and show an error.
    1. SharedUX team (@tsullivan) implements the fixes using separate PRs
  2. [In this PR] Soften the error behavior and take effect only in Dev mode
    1. Use console.error to give an error message and a stack trace
    2. Show an Error toast to convey the error message to Developers
  3. [Next] After running in a mode of step 2 for a length of time, we add restrictions to CI environments with the final error behavior. Update the navigation methods of the common page objects for functional tests, to detect when the toast message appears during functional testing, and throw a helpful error.

Notification for developers

Developers would see this error toast with a message telling them they need to use EuiProvider, plus a link for "more details." They can also check the JS console to see the error details.

image

How this helps

Surfacing these kinds of errors automates the discovery of finding where dark mode issues exist. Here is an example of a hard-to-find dark mode issue that was easily caught by the kinds of testing enabled in this PR:

connection details - before

The way to problems in this nature is to wrap the React context in <KibanaRenderContextProvider>, and setting the required CoreStart service dependencies of analytics, i18n and theme.

@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) labels May 31, 2024
@tsullivan tsullivan force-pushed the euiprovider/detect-issues branch 4 times, most recently from a13dff6 to 15141dc Compare July 8, 2024 21:21
notifications.toasts.addDanger({
title: '`EuiProvider` is missing',
text: mountReactNode(
<p data-test-sub="core-chrome-euiDevProviderWarning-toast">
Copy link
Member Author

@tsullivan tsullivan Jul 22, 2024

Choose a reason for hiding this comment

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

With this data-test-subj, there could be a change in another PR to catch scenarios when this unwanted toast shows in functional tests. We could update the navigation methods of CommonPageObject class, to catch this toast message during functional testing, and throw a helpful error.

Copy link
Member Author

Choose a reason for hiding this comment

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

@tsullivan tsullivan marked this pull request as ready for review July 22, 2024 23:00
@tsullivan tsullivan requested a review from a team as a code owner July 22, 2024 23:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@tsullivan tsullivan force-pushed the euiprovider/detect-issues branch from 8ee9364 to 535fe00 Compare July 22, 2024 23:00
@tsullivan tsullivan self-assigned this Jul 22, 2024
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

🚀

@tsullivan tsullivan enabled auto-merge (squash) July 23, 2024 18:44
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #101 / Screenshots - serverless security UI response ops docs security cases security case settings case settings screenshot

Metrics [docs]

Page load bundle

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

id before after diff
core 415.3KB 416.0KB +765.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/core-chrome-browser-internal 2 3 +1

Total ESLint disabled count

id before after diff
@kbn/core-chrome-browser-internal 2 3 +1

History

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

cc @tsullivan

@tsullivan tsullivan merged commit f43b5d5 into elastic:main Jul 23, 2024
21 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 23, 2024
@tsullivan tsullivan deleted the euiprovider/detect-issues branch July 24, 2024 19:48
TinLe added a commit to TinLe/kibana that referenced this pull request Jul 30, 2024
* master: (3487 commits)
  `BedrockChat` & `GeminiChat` (elastic#186809)
  [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332)
  skip flaky suite (elastic#188997)
  [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580)
  [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608)
  [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943)
  Improve SearchSource SearchRequest type (elastic#186862)
  Deprecate Search Sessions config (elastic#188037)
  [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824)
  [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746)
  [Data Forge] Add `service.logs` dataset as a  data stream (elastic#188786)
  [Console] Fix failing bulk requests (elastic#188552)
  Update dependency terser to ^5.31.2 (main) (elastic#188528)
  [APM][ECO] Telemetry (elastic#188627)
  [Fleet] Fix uninstall package validation accross space (elastic#188749)
  Update warning on `xpack.fleet.enableExperimental` (elastic#188917)
  [DOCS][Cases] Automate more screenshots for cases (elastic#188697)
  [Fleet] Fix get one agent when feature flag disabled (elastic#188953)
  chore(investigate): Add investigate-app plugin from poc (elastic#188122)
  [Monaco Editor] Add Search functionality (elastic#188337)
  ...
TinLe added a commit to TinLe/kibana that referenced this pull request Jul 30, 2024
* master: (2400 commits)
  `BedrockChat` & `GeminiChat` (elastic#186809)
  [ResponseOps] log error when ES Query rules find docs out of time range (elastic#186332)
  skip flaky suite (elastic#188997)
  [Security solution][Alert Details] Enable preview feature flag and cypress tests (elastic#188580)
  [EuiProviders] Warn Developer if EuiProvider is missing (elastic#184608)
  [Security Solution ] Fixes Timeline infinite loading bug (elastic#188943)
  Improve SearchSource SearchRequest type (elastic#186862)
  Deprecate Search Sessions config (elastic#188037)
  [Synthetics] Add missing monitorType and tag info in cards !! (elastic#188824)
  [Console Monaco] Resolve uncaught error from tokenizer (elastic#188746)
  [Data Forge] Add `service.logs` dataset as a  data stream (elastic#188786)
  [Console] Fix failing bulk requests (elastic#188552)
  Update dependency terser to ^5.31.2 (main) (elastic#188528)
  [APM][ECO] Telemetry (elastic#188627)
  [Fleet] Fix uninstall package validation accross space (elastic#188749)
  Update warning on `xpack.fleet.enableExperimental` (elastic#188917)
  [DOCS][Cases] Automate more screenshots for cases (elastic#188697)
  [Fleet] Fix get one agent when feature flag disabled (elastic#188953)
  chore(investigate): Add investigate-app plugin from poc (elastic#188122)
  [Monaco Editor] Add Search functionality (elastic#188337)
  ...
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 release_note:skip Skip the PR/issue when compiling release notes Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants