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

[meta] Adapt all existing applications to use the new KibanaThemeProvider #118866

Closed
pgayvallet opened this issue Nov 17, 2021 · 21 comments
Closed
Labels
EUI impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Meta Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v8.1.0

Comments

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 17, 2021

Context

For the css-in-js initiative, the EUI team recently added a new theme context provider: EuiThemeProvider (see #108972 and elastic/eui#4511).

To have Kibana be ready to use the features related to this theme provider, we need all react applications to wrap their root component / react trees with it. Ideally, this would have been done exclusively at core's level, however, due to the agnostic nature of core's apps, and the fact that applications are effectively distinct react trees (e.g calling ReactDOM.render during their mount), we will need all application owners to adapt their application mounting points.

Also, applications are not the only parts of Kibana rendering react trees. For example, all usage of kibana_react's toMountPoint also need to be adapted, as, for example, when calling core.overlays.openFlyout(mountPoint). Management sections needs to be adapted too.

EUI is planning on leveraging the theme provider to carry the reset styles in their next phase, for 8.1, so this has to be done for the 8.1 FF

How do we do that

In #117368 we added a new theme core service, and created the <KibanaThemeProvider /> component and wrapWithTheme utility that are exposed from the kibana_react plugin. We also adapted toMountPoint to accept the theme$ as an optional parameter to wrap the provided component with the theme provider.

To summarize, all react trees (all root components passed to ReactDOM.render) must now be wrapped with <KibanaThemeProvider>, either directly, or by using wrapWithTheme.

ReactDOM.render

For applications

To ease the integration, a new theme$ option has been added to AppMountParameters, to help using KibanaThemeProvider

before

import React from 'react';
import ReactDOM from 'react-dom';
import { I18nProvider } from '@kbn/i18n/react';
import type { AppMountParameters } from 'src/core/public';

export const renderApp = ({ element }: AppMountParameters) => {
  ReactDOM.render(
    <I18nProvider>
      <MyApp />
    </I18nProvider>,
    element
  );

  return () => {
    ReactDOM.unmountComponentAtNode(element);
  };
};

after

import React from 'react';
import ReactDOM from 'react-dom';
import { I18nProvider } from '@kbn/i18n/react';
import type { AppMountParameters } from 'src/core/public';
import { KibanaThemeProvider } from '../../kibana_react/public';

export const renderApp = ({ element, theme$ }: AppMountParameters) => {
  ReactDOM.render(
    <I18nProvider>
      <KibanaThemeProvider theme$={theme$}>
        <MyApp />
      </KibanaThemeProvider>
    </I18nProvider>,
    element
  );

  return () => {
    ReactDOM.unmountComponentAtNode(element);
  };
};

For management section.

This is pretty much the same. theme$ has been added to ManagementAppMountParams in #118852

Also see https://github.com/elastic/kibana/pull/117368/files#diff-8665e34e28fd55dda21b54f5d3aeecf5393ba92705295192f18e91ae85298513 for an example of the savedObjects management section before theme$ was exposed to the management mount parameters.

Other usages of ReactDOM.render

For other usages of render, the theme$ will have to be manually retrieved to be passed down to <KibanaThemeProvider>. It can be accessed via coreSetup.theme.theme$ or coreStart.theme.theme$;

toMountPoint

toMountPoint has been adapted to accept an optional theme$ option as a property of its second parameter. However, due to the fact that this function can be called pretty much anywhere in the code, we couldn't ease the integration as we did by adding the theme$ property to AppMountParameters and ManagementAppMountParams, so propagating the theme down to were the call to toMountPoint is performed will have to be done manually.

// old signature
export const toMountPoint = (node: React.ReactNode): MountPoint
// new signature
export const toMountPoint = (node: React.ReactNode, { theme$ }: ToMountPointOptions = {}): MountPoint

this theme$ option will need to be used for all usages of toMountPoint, to make sure the theme is properly propagated

before

overlays.openModal(
  toMountPoint(
    <MyModal/>
  )
);

after

overlays.openModal(
  toMountPoint(
    <MyModal/>,
    { theme$: core.theme.theme$ }
  )
);

or

overlays.openModal(
  toMountPoint(
    wrapWithTheme(<MyModal/>, { theme$: core.theme.theme$ }),
  )
);

Applications (and management applications) by team

@elastic/kibana-core

applications

ApplicationId PluginId Status PR
status core 🟢 #117368
error core 🟢 #117368
home home 🟢 #119254
kibanaOverview kibanaOverview 🟢 #118881

management apps

ApplicationId SectionId Status PR
objects kibana 🟢 #117368
tags kibana 🟢 #121827

@elastic/kibana-vis-editors

PR: #119298

applications

ApplicationId PluginId Status PR
visualize visualize 🟢
lens lens 🟢

management apps

ApplicationId SectionId Status PR
settings kibana 🟢

@elastic/kibana-stack-management

applications

ApplicationId PluginId Status PR
dev_tools devTools 🟢 #120003
console devTools 🟢 #120003
searchprofiler devTools 🟢 #120003
grokdebugger devTools 🟢 #120003
painless_lab devTools 🟢 #120003
management management 🟢 #120003

management apps

ApplicationId SectionId Status PR
license_management stack 🟢 #120003
cross_cluster_replication data 🟢 #120003
watcher insightsAndAlerting 🟢 #120003
remote_clusters data 🟢 #120003
rollup_jobs data 🟢 #120003
snapshot_restore data 🟢 #120003
ingest_pipelines ingest 🟢 #120003
index_management data 🟢 #120003
index_lifecycle_management data 🟢 #120003
upgrade_assistant stack 🟢 #120003

@elastic/logstash

management apps

ApplicationId SectionId Status PR
pipelines ingest 🟢 #122311

@elastic/kibana-app-services #119628

applications

ApplicationId PluginId Status PR
short_url_redirect share 🟢
r share 🟢
reportingRedirect reporting 🟢

management apps

ApplicationId SectionId Status PR
dataViews kibana 🟢
reporting insightsAndAlerting 🟢
search_sessions kibana 🟢

@elastic/kibana-security

applications

ApplicationId PluginId Status PR
interactiveSetup interactive_setup 🟢 #119124
space_selector spaces 🟢 #119124
security_access_agreement security 🟢 #119124
security_capture_url security N/A * N/A *
security_login security 🟢 #119124
security_logout security N/A * N/A *
security_logged_out security 🟢 #119124
security_overwritten_session security 🟢 #119124
security_account security 🟢 #119124

* This app doesn't actually render anything, it just exists to redirect the user somewhere else for authN purposes

management apps

ApplicationId SectionId Status PR
spaces kibana 🟢 #119124
users security 🟢 #119124
roles security 🟢 #119124
api_keys security 🟢 #119124
role_mappings security 🟢 #119124

@elastic/kibana-presentation

applications

ApplicationId PluginId Status PR/Issue
canvas canvas 🟢 #120071
dashboards dashboard 🟢 #120122

@elastic/kibana-data-discovery

applications

ApplicationId PluginId Status PR
graph graph 🟢 #119802
discover discover 🟢 #119784

@elastic/fleet #119250

applications

ApplicationId PluginId Status PR
integrations fleet 🟢 #122385
fleet fleet 🟢 #122385
ingestManager fleet 🟢 n/a (redirects to fleet)

@elastic/kibana-gis

applications

ApplicationId PluginId Status PR
maps maps 🟢 maps-app #119060 , maps-embeddable #118875

@elastic/observability-ui

applications

ApplicationId PluginId Status PR
observability-overview observability 🟢 #120400, #120968

@elastic/security-asset-management

applications

ApplicationId PluginId Status PR
osquery osquery 🟢 #122051

@elastic/ml-ui

applications

ApplicationId PluginId Status PR
ml ml 🟢 #120892
data_visualizer data_visualizer 🟢 #121286

management apps

ApplicationId SectionId Status PR
transform data 🟢 #120933
jobsListLink insightsAndAlerting 🟢 #120892

@elastic/uptime

applications

ApplicationId PluginId Status PR
uptime uptime 🟢 #118965

@elastic/security-solution

applications

ApplicationId PluginId Status PR
securitySolutionUI securitySolution 🟢 #123499
siem securitySolution 🟢 #123499

@elastic/infra-monitoring-ui

applications

ApplicationId PluginId Status PR
logs infra 🟢 #120164
metrics infra 🟢 #120164
infra infra 🟢 #120164
monitoring monitoring 🟢 #120713

@elastic/enterprise-search-frontend

applications

ApplicationId PluginId Status PR
enterpriseSearch enterpriseSearch 🟢 #120244
appSearch enterpriseSearch N/A* N/A
workplaceSearch enterpriseSearch N/A* N/A
  • App Search and Workplace Search share the same renderApp method as Enterprise Search

@elastic/apm-ui

applications

ApplicationId PluginId Status PR
apm apm 🟢 #118869
ux apm 🟢

@elastic/kibana-alerting-services

#118880

management apps

ApplicationId SectionId Status PR
triggersActions insightsAndAlerting 🟢 #121415
@pgayvallet pgayvallet added Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Meta EUI labels Nov 17, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-design (Team:Kibana-Design)

@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-design (EUI)

@sorenlouv sorenlouv added Team:APM All issues that need APM UI Team support and removed Team:APM All issues that need APM UI Team support labels Nov 17, 2021
@cchaos cchaos added the v8.1.0 label Nov 17, 2021
@jportner
Copy link
Contributor

@pgayvallet when working on implementing this for Platform Security in #119124, we noticed that nav control registration (core.chrome.navControls.register*) seem to have been overlooked in this meta-issue.

We're updating the Security and Spaces consumption of this in our PR, but I wanted to post here that it looks like there is other usage that needs to be updated, in case you want to tag affected teams in this meta-issue:

@pgayvallet
Copy link
Contributor Author

@jportner thanks for the heads up, you're right, I did not do an exhaustive list of APIs where render is used 'downstream'. I will take a look of the navControls APIs.

@matschaffer
Copy link
Contributor

Pretty sure @elastic/observability-ui is done (prs are merged) so I marked that as green.

@pgayvallet
Copy link
Contributor Author

Thanks to everyone for working on this.

I still see a few teams that don't seem to have opened issues or PRs yet to track their part of the task:

  • @elastic/kibana-app-services
  • @elastic/fleet
  • @elastic/kibana-gis
  • @elastic/security-solution

For those teams, even if you did not yet start working on this issue, could you please link your section to either a PR or an issue so that I can track the remaining requires changes? Would be greatly appreciated.

@joshdover
Copy link
Contributor

I've added our tracking issue (#119250) to the description for the Fleet team. Work has already begun, should have a PR up soon.

@dhurley14
Copy link
Contributor

security solution is tracking this with this issue: #119320

I don't believe work has begun on this yet.

@dhurley14
Copy link
Contributor

security solution should be covered now that this PR has merged: #123499

@pgayvallet
Copy link
Contributor Author

We're almost there. Only team remaining is @elastic/apm-ui / #118869

@pgayvallet
Copy link
Contributor Author

All teams have completed their tasks. Closing.

@dikshachauhan-qasource
Copy link

Hi @pgayvallet

Could you please provide us more details on how this ticket changes will impact Kibana UI. So that we can work upon validating the same.

Thanks
QAS

@pgayvallet
Copy link
Contributor Author

@dikshachauhan-qasource This is mostly a technical change that shouldn't impact the UI in any way. The only test that could potentially be performed is to make sure that light/dark mode is still working as intended in all applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EUI impact:needs-assessment Product and/or Engineering needs to evaluate the impact of the change. loe:small Small Level of Effort Meta Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. v8.1.0
Projects
None yet
Development

No branches or pull requests