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] Use KibanaThemeProvider in ML plugin #120892

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Dec 9, 2021

Summary

Adapts the ML plugin and the ML jobs page under Stack Management to use the KibanaThemeProvider implementation and the theme$ prop in toMountPoint calls.

Part of #119007 and #118866.

Checklist

@peteharverson peteharverson added review chore :ml release_note:skip Skip the PR/issue when compiling release notes v8.1.0 labels Dec 9, 2021
@peteharverson peteharverson requested a review from a team as a code owner December 9, 2021 12:29
@peteharverson peteharverson self-assigned this Dec 9, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

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.

Code LGTM

@peteharverson
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion about the dependency cache

@@ -20,8 +20,15 @@ export function showExpiredLicenseWarning() {
});
// Only show the banner once with no way to dismiss it
const overlays = getOverlays();
const theme = getTheme();
Copy link
Contributor

Choose a reason for hiding this comment

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

We actually can avoid using dependency cache here by passing overlays and a theme as arguments. First to MlClientLicense constructor and then to this function.

Copy link
Member

Choose a reason for hiding this comment

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

MlClientLicense would need to keep a local reference to overlays and theme for use later, which is no different to the dependency cache keeping references to them.
At least with the dependency cache they are all in the same place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. @jgowdyelastic and I discussed this and decided that keeping a copy in the cache rather than a copy in the licence is simpler for now.

@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
ml 3.5MB 3.5MB +726.0B

Page load bundle

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

id before after diff
ml 38.3KB 38.5KB +162.0B

History

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

cc @peteharverson

@peteharverson peteharverson merged commit 32d0e87 into elastic:main Dec 13, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 13, 2021
@peteharverson peteharverson deleted the ml-kibana-theme-provider branch December 13, 2021 10:45
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
gbamparop pushed a commit to gbamparop/kibana that referenced this pull request Jan 12, 2022
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 :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.

6 participants