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

[ML] Data visualizer: Use KibanaThemeProvider #121286

Merged

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Dec 15, 2021

Summary

Adapts the data_visualizer plugin to use the KibanaThemeProvider implementation.

Also contains a fix for the document count chart when viewing inside the ML plugin in dark theme, where the mouseover highlight wasn't adapting to the theme - the theme is now passed to the elastic chart in the Settings section.

Before:
dataviz_dark_theme_before

After:
image

Part of #119007 and #118866.

Checklist

@peteharverson peteharverson added review chore :ml release_note:skip Skip the PR/issue when compiling release notes Feature:File and Index Data Viz ML file and index data visualizer backport:skip This commit does not require backporting v8.1.0 labels Dec 15, 2021
@peteharverson peteharverson self-assigned this Dec 15, 2021
@peteharverson peteharverson requested a review from a team as a code owner December 15, 2021 11:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@@ -37,6 +38,7 @@ export interface DataVisualizerStartDependencies {
security?: SecurityPluginSetup;
share: SharePluginStart;
lens?: LensPublicStart;
charts?: ChartsPluginStart;
Copy link
Member

@jgowdyelastic jgowdyelastic Dec 15, 2021

Choose a reason for hiding this comment

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

charts is now a required plugin and so doesn't need to be optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 69c0bd3

} = useDataVisualizerKibana();

const chartTheme = charts?.theme.useChartsTheme();
Copy link
Member

Choose a reason for hiding this comment

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

Making charts non optional as per my previous comment means we can lose the ? here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 69c0bd3

@qn895
Copy link
Member

qn895 commented Dec 15, 2021

Tested and LGTM 🎉

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
dataVisualizer 536.3KB 536.6KB +303.0B

History

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

cc @peteharverson

@peteharverson peteharverson merged commit b2757cd into elastic:main Dec 16, 2021
@peteharverson peteharverson deleted the data-vis-kibana-theme-provider branch December 16, 2021 09:29
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* [ML] Data visualizer: Use KibanaThemeProvider

* [ML] Edits following review
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
* [ML] Data visualizer: Use KibanaThemeProvider

* [ML] Edits following review
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 chore Feature:File and Index Data Viz ML file and index data visualizer :ml release_note:skip Skip the PR/issue when compiling release notes review v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants