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

[Maps] Use theme provider in Maps-app #119060

Merged
merged 3 commits into from
Nov 30, 2021

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Nov 18, 2021

Part of #118875

@thomasneirynck thomasneirynck requested a review from a team as a code owner November 18, 2021 17:33
@thomasneirynck thomasneirynck added chore release_note:skip Skip the PR/issue when compiling release notes [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.1.0 labels Nov 18, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

What about map_embeddable? Is KibanaThemeProvider needed there as well?

@thomasneirynck
Copy link
Contributor Author

@nreese wrt #119060 (review)

What about map_embeddable? Is KibanaThemeProvider needed there as well?

I think not, because the embeddable will be inside the tree of other Kibana apps, and presumably we don't want duplicated nesting of this. This is an assumption only though. @pgayvallet is that correct?

@pgayvallet
Copy link
Contributor

pgayvallet commented Nov 22, 2021

What about map_embeddable? Is KibanaThemeProvider needed there as well?

Given that map_embeddable uses it's own ReactDOM.render, it will need to be wrapper too, as it's a distinct react tree (yea, implementing 'global' providers in our architecture is a pain...).

Note that ideally, the theme$ passed down to MapEmbeddable would be provided by the 'owning' app though, as it would make more sense that have it provided internally by the embeddable plugin, but we can live without it for now if that's too complex.

@nreese
Copy link
Contributor

nreese commented Nov 24, 2021

With respect to #118866 (comment), looks like there are 4 places with Theme provider is required in maps plugin

Screen Shot 2021-11-24 at 11 01 06 AM

@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Nov 29, 2021

@pgayvallet

wrt #119060 (comment)

If the "owning-app" needs to inject the $theme, I'd prefer splitting up this PR in different ones. There are quite a few "owning"-plugins that embed maps, all of them requiring different reviewing teams.

  • Dashboard
  • APM rum dashboards
  • SIEM
  • Canvas
  • Tilemap legacy wrapper
  • Regionmap legacy wrapper
  • ML data visualizer
  • ML anomaly explorer

For sanity, it might be easier to address them one-by-one (or, at least, in groups that make sense).

A parallel question: can we wrap the dom-trees with the kb-theme-provider without configuring the $theme-variable?

@thomasneirynck
Copy link
Contributor Author

@elasticmachine merge upstream

@thomasneirynck thomasneirynck changed the title [Maps] Use theme provider [Maps] Use theme provider in Maps-app Nov 30, 2021
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM - discussed that this is part of the fix and does not provide theme in all required locations
code review

@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
maps 2.6MB 2.6MB +59.0B

History

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

@thomasneirynck thomasneirynck merged commit a7d8ee6 into elastic:main Nov 30, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 30, 2021
@pgayvallet
Copy link
Contributor

A parallel question: can we wrap the dom-trees with the kb-theme-provider without configuring the $theme-variable?

Sorry, missed that one. It wouldn't make much sense, given that we wouldn't be propagating the required variables to the EUI theme provider.

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 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants