[drilldown] fix Darktheme is missing from add drilldowns panel #147270
Conversation
nreese
added
release_note:fix
Team:Presentation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
Thanks. I have resolved this with 78b1b97 |
|
@elasticmachine merge upstream |
| this.ReactCollectConfig = (props) => <CollectConfigContainer {...props} params={this.params} />; | ||
| constructor(protected readonly params: Params, theme: ThemeServiceStart) { | ||
| this.ReactCollectConfig = (props) => ( | ||
| <KibanaThemeProvider theme$={theme.theme$}> |
There was a problem hiding this comment.
I think we have the same issue in x-pack/plugins/lens/public/trigger_actions/open_in_discover_drilldown.tsx and x-pack/plugins/drilldowns/url_drilldown/public/lib/url_drilldown.tsx
We introduced these uiToReact and reactToUi conversions because we wanted our APIs to be React agnostic, but I wonder if in 2 years this opinion has changed it is better to just use plain React.
Then it a way to fix this and simplify is to remove intermediate non-react conversion steps.
@vadimkibana, wdyt?
Of course we can also fix this theming issue by re-wrapping each instance in KibanaThemeProvider or do it inside reactToUiComponent helper and pass theme.theme$ there
There was a problem hiding this comment.
We introduced these uiToReact and reactToUi conversions because we wanted our APIs to be React agnostic, but I wonder if in 2 years this opinion has changed it is better to just use plain React.
+1 to remove these conversions and just use plain react.
There was a problem hiding this comment.
+1 to just use plain react.
There was a problem hiding this comment.
+1 as well for using React directly without breaking the React context. Historically at Kibana there was a trend to abstract everything away from a rendering library, e.g. keep Kibana agnostic to the view implementation. But in general, I believe, we are reversing this to simplify code everywhere.
@nreese maybe in ui_actions where UiComponent is required you could also allow for React.FunctionalComponent as well, and then we don't need this conversion and drilldown configurator can check if the thing is already a React component, then just use it directly.
nreese
force-pushed
the
drilldown_darkmode2
branch
from
2dec1ee
to
104d4f9
Compare
|
#148037 has merged and removes UiComponent. #148037 resolves the dark theme issues within the drill down panel. I have aborted changes made in x-pack/plugins/dashboard_enhanced/public/services/drilldowns/abstract_dashboard_drilldown/abstract_dashboard_drilldown.tsx since the are no longer needed. I am removing 8.6 label since #148037 is only merged into 8.7. I would rather leave this issue unresolved in 8.6 and resolve cleanly in 8.7, then have less clean solution in for 8.6 that adds additional ThemeProviders wrappers that then have to be clean-up again in 8.7 |
nreese
added
backport:skip
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
…ic#147270) Fixes elastic#147147 Replaces elastic#147151. Thanks for the tip @Dosant PR passes theme$ to toMountPoint <img width="400" alt="Screen Shot 2022-12-06 at 4 54 49 PM" src="https://user-images.githubusercontent.com/373691/206051509-207c39f6-125d-46f9-baab-87a5a96a6541.png"> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Fixes #147147
Replaces #147151. Thanks for the tip @Dosant
PR passes theme$ to toMountPoint