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/ResponseOps] Remove usage of deprecated React rendering utilities #180098

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Apr 4, 2024

Summary

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

Follows #180003

These changes come up from searching in the code and finding where certain kinds of deprecated AppEx-SharedUX modules are imported. Reviewers: Please interact with critical paths through the UI components touched in this PR, ESPECIALLY in terms of testing dark mode and i18n.

  • Started with focusing on code owned by ResponseOps.
  • Stack Management changes trickled into this PR because ResponseOps needs the theme field from ManagementAppMountParams
  • Security changes trickled into this PR, in its test code, because of the Stack Management changes.

Checklist

Delete any items that are not applicable to this PR.

@tsullivan tsullivan force-pushed the sharedux/cleanup-tomountpoint-deprecations-ii branch 2 times, most recently from 6584509 to df6aa4d Compare April 4, 2024 23:54
@@ -114,30 +108,28 @@ export const ManagementApp = ({
};

return (
<RedirectAppLinks coreStart={dependencies.coreStart}>
<I18nProvider>
<KibanaRenderContextProvider i18n={coreStart.i18n} theme={coreStart.theme}>
Copy link
Member Author

Choose a reason for hiding this comment

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

I placed KibanaRenderContextProvider above other context providers, in all changes, for consistency

@@ -93,16 +93,12 @@ export const renderApp = ({
}}
>
<Router history={history}>
<EuiThemeProvider darkMode={isDarkMode}>
<i18nCore.Context>
Copy link
Member Author

Choose a reason for hiding this comment

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

The i18n context provider is not explicitly needed since KibnaRenderContextProvider provides it

@@ -30,16 +29,15 @@ const ActionWrapperWithContext: React.FC<PropsWithChildren<Props>> = ({
caseContextProps,
currentAppId,
}) => {
const { application } = useKibana().services;
const isDarkTheme = useIsDarkTheme();
Copy link
Member Author

Choose a reason for hiding this comment

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

useIsDarkTheme seems to just be passing through the dark theme setting from core, which isn't explicitly needed with KibanaRenderContextProvider


setDataViewsService(dataViews);
return (
<I18nProvider>
Copy link
Member Author

Choose a reason for hiding this comment

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

The i18n context provider is not explicitly needed since KibnaRenderContextProvider provides it

@@ -29,23 +27,18 @@ export const renderApp = (deps: TriggersAndActionsUiServices) => {
};

export const App = ({ deps }: { deps: TriggersAndActionsUiServices }) => {
const { dataViews, theme, theme$ } = deps;
const isDarkMode = theme.getTheme().darkMode;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just passing through the dark mode setting from Core, which isn't explicitly needed with KibanaRenderContextProvider


const sectionsRegex = sections.join('|');
setDataViewsService(dataViews);
return (
<I18nProvider>
Copy link
Member Author

Choose a reason for hiding this comment

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

The i18n context provider is not explicitly needed since KibnaRenderContextProvider provides it

return render(<ModalInspectQuery {...defaultProps} />, {
wrapper: ({ children }) => <EuiThemeProvider>{children}</EuiThemeProvider>,
wrapper: ({ children }) => (
<KibanaThemeProvider theme={theme}>{children}</KibanaThemeProvider>
Copy link
Member Author

Choose a reason for hiding this comment

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

Not using KibanaRenderContextProvider here since this is a test file

@tsullivan tsullivan force-pushed the sharedux/cleanup-tomountpoint-deprecations-ii branch 2 times, most recently from 39699cf to 95376ef Compare April 5, 2024 20:20
@tsullivan tsullivan force-pushed the sharedux/cleanup-tomountpoint-deprecations-ii branch from 4d5e7d0 to 5c69f2c Compare April 5, 2024 23:48
@tsullivan
Copy link
Member Author

/ci

bulkDisableRules,
bulkEnableRules,
bulkDeleteRules,
snoozeRule,
unsnoozeRule,
Copy link
Member Author

@tsullivan tsullivan Apr 6, 2024

Choose a reason for hiding this comment

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

removed unused variables to eliminate warnings seen in the editor

@tsullivan tsullivan force-pushed the sharedux/cleanup-tomountpoint-deprecations-ii branch from cbe06c8 to 2578f8d Compare April 6, 2024 03:27
@tsullivan tsullivan marked this pull request as ready for review April 6, 2024 03:27
@tsullivan tsullivan requested review from a team as code owners April 6, 2024 03:27
@tsullivan tsullivan added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes labels Apr 6, 2024
@@ -76,8 +69,8 @@ export const getMockApplications$ = () =>

export const getMockCaseUiActionProps = () => {
const core = {
...coreStart,
Copy link
Member Author

Choose a reason for hiding this comment

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

This was done to add i18n to the context

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 164 174 +10
cases 714 717 +3
management 117 127 +10
triggersActionsUi 699 713 +14
total +37

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
management 43 44 +1
triggersActionsUi 566 567 +1
total +2

Async chunks

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

id before after diff
alerting 90.7KB 91.5KB +917.0B
cases 447.5KB 446.5KB -1.0KB
management 43.5KB 44.5KB +1019.0B
triggersActionsUi 1.6MB 1.6MB +11.7KB
total +12.6KB

Page load bundle

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

id before after diff
alerting 23.7KB 23.6KB -17.0B
cases 147.7KB 148.9KB +1.1KB
triggersActionsUi 116.6KB 116.7KB +120.0B
total +1.2KB
Unknown metric groups

API count

id before after diff
management 43 44 +1
triggersActionsUi 592 593 +1
total +2

References to deprecated APIs

id before after diff
alerting 119 116 -3
cases 19 14 -5
cloudDataMigration 3 4 +1
crossClusterReplication 5 6 +1
indexLifecycleManagement 5 6 +1
indexManagement 6 7 +1
ingestPipelines 4 5 +1
licenseManagement 5 6 +1
logstash 5 6 +1
management 4 1 -3
remoteClusters 2 3 +1
reporting 34 35 +1
rollup 5 6 +1
savedObjectsTagging 45 46 +1
security 66 70 +4
snapshotRestore 5 6 +1
spaces 22 23 +1
triggersActionsUi 53 26 -27
upgradeAssistant 9 10 +1
watcher 6 7 +1
total -19

History

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

@tsullivan tsullivan self-assigned this Apr 9, 2024
@@ -106,7 +106,8 @@ const getErrorMessage = (error: Error | ServerError): string => {

export const useCasesToast = () => {
const { appId } = useApplication();
const { getUrlForApp, navigateToUrl } = useKibana().services.application;
const { application, i18n, theme } = useKibana().services;
Copy link
Member

Choose a reason for hiding this comment

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

Are these services available to all plugins? I am asking because Cases are used in a lot of places across Kibana.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cnasikas these services are available to all plugins as part of the CoreStart contract. They are coming from this useKibana() hook as they are provided to its context provider here.

Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks!

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Changes in Security plugin tests LGTM, thanks!

@ElenaStoeva ElenaStoeva self-requested a review April 9, 2024 09:49
Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

Code looks better ;) and I tested it locally and everything seem to work is working as expected

image

Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Management changes LGTM

@tsullivan tsullivan merged commit 09228a3 into elastic:main Apr 9, 2024
36 checks passed
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 9, 2024
@tsullivan tsullivan deleted the sharedux/cleanup-tomountpoint-deprecations-ii branch April 9, 2024 16:31
tsullivan added a commit that referenced this pull request Apr 16, 2024
…180521)

## Summary

Partially addresses elastic/kibana-team#805

Follows #180098

These changes come up from searching in the code and finding where
certain kinds of deprecated AppEx-SharedUX modules are imported.
**Reviewers: Please interact with critical paths through the UI
components touched in this PR, ESPECIALLY in terms of testing dark mode
and i18n.**

This focuses on code within Response Ops.

<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 <[email protected]>
@tsullivan tsullivan changed the title [ResponseOps] Remove usage of deprecated React rendering utilities [EuiProvider/ResponseOps] Remove usage of deprecated React rendering utilities Jul 20, 2024
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 technical debt Improvement of the software architecture and operational architecture v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants