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

[EuiProvider] Fix Security Solution code #183878

Merged
merged 4 commits into from
May 24, 2024

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented May 20, 2024

Summary

Fixes needed for getting CI to pass when EUI throws an error if attempting to render a component without the EuiProvider in the render tree.

Detailed description

In #180819, I will deliver a change that will cause EUI components to throw an error if the EuiProvider context is missing. This PR comes in as part of the final work to get all functional tests passing in an environment where EUI will throw the error. The tied to the "Fix 'dark mode' inconsistencies in Kibana" Epic has so far been in preparation for this.

Reviewers: Please interact with critical paths through the UI components touched in this PR, ESPECIALLY in terms of testing dark mode and i18n.

image

Checklist

Delete any items that are not applicable to this PR.

@tsullivan
Copy link
Member Author

/ci

@tsullivan tsullivan marked this pull request as ready for review May 21, 2024 19:02
@tsullivan tsullivan requested review from a team as code owners May 21, 2024 19:02
@tsullivan tsullivan added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels May 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@tsullivan tsullivan self-assigned this May 21, 2024
Copy link
Contributor

@patrykkopycinski patrykkopycinski left a comment

Choose a reason for hiding this comment

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

Thank you @tsullivan

Copy link
Contributor

@nikitaindik nikitaindik left a comment

Choose a reason for hiding this comment

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

Greetings @tsullivan! I have tested the changes and haven't found any issues. I changed the language and turned the dark mode on – both do get applied to the modal.

Scherm­afbeelding 2024-05-23 om 11 58 14

I've also noticed that "mountReactNode" has its own "I18nProvider", so the content ends up being wrapped twice.

Scherm­afbeelding 2024-05-23 om 11 58 44

I wonder if it would be possible to move KibanaRenderContextProvider inside mountReactNode. If we manage to do that, then we won't have to pass startServices here and will potentially cover more places at once. wdyt?

@tsullivan
Copy link
Member Author

@nikitaindik great catch, and worthy of discussion. I would propose switching usage of { mountReactNode } from '@kbn/core-mount-utils-browser-internal' with { toMountPoint } from '@kbn/react-kibana-mount'. I'd prefer if mountReactNode was deprecated or only used in Core, if needed. We seem to have too many redundant utilities that do the same thing but slightly differently.

The file in question is security_solution/public/detection_engine/rule_details_ui/pages/rule_details/execution_log_table/execution_log_table.tsx. If we import toMountPoint there, we still have to pass startServices but it solves the issue of having 2 wrappers for i18n.

I put up a commit to do this, for your re-review: 97e75f8

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5490 5483 -7

Async chunks

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

id before after diff
securitySolution 15.1MB 15.1MB -779.0B

History

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

cc @tsullivan

Copy link
Contributor

@nikitaindik nikitaindik 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 adding the change @tsullivan! I have re-tested it locally and haven't found any issues. Can confirm that there are no duplication of I18nProvider.

@tsullivan tsullivan merged commit f1129e3 into elastic:main May 24, 2024
36 checks passed
@tsullivan tsullivan deleted the euiprovider/fix-security-solution branch May 24, 2024 16:11
@kibanamachine kibanamachine added v8.15.0 backport:skip This commit does not require backporting labels May 24, 2024
rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024
## Summary

Fixes needed for getting CI to pass when EUI throws an error if
attempting to render a component without the EuiProvider in the render
tree.

## Detailed description
In elastic#180819, I will deliver a
change that will cause EUI components to throw an error if the
EuiProvider context is missing. This PR comes in as part of the final
work to get all functional tests passing in an environment where EUI
will throw the error. The tied to the ["Fix 'dark mode' inconsistencies
in Kibana" Epic](elastic/kibana-team#805) has
so far been in preparation for this.

**Reviewers: Please interact with critical paths through the UI
components touched in this PR, ESPECIALLY in terms of testing dark mode
and i18n.**

<img width="1107" alt="image"
src="https://github.com/elastic/kibana/assets/908371/c0d2ce08-ac35-45a7-8192-0b2256fceb0e">

### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants