[Observability AI Assistant] Fix dark mode#179716
Conversation
|
Pinging @elastic/obs-knowledge-team (Team:obs-knowledge) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
| core.theme.theme$.subscribe(async (theme) => { | ||
| const Component = await getEditLensConfiguration(core, deps, visualizationMap, datasourceMap); | ||
| const ConfigPanel = ( | ||
| <EuiThemeProvider colorMode={theme.darkMode ? 'dark' : 'light'}> |
There was a problem hiding this comment.
This is the change to get the embeddable to render correctly in dark mode when no container is passed (this particular code path).
If there is a different theming API or approach that is better suited for this use case, please lmk.
There was a problem hiding this comment.
Maybe is better to add this here https://github.com/elastic/kibana/blob/main/x-pack/plugins/lens/public/app_plugin/shared/edit_on_the_fly/get_edit_lens_configuration.tsx#L226 instead of the action.
This dark mode inconsistency doesnt happen to dashboard or Discover. Are you wrapping the Obs AI assistant with the ThemeProvider? 🤔
Maybe you do and the problem happens because we are getting the component with the action I just wonder. If this is the case and we need to add the provider to Lens let's do it on the get_edit_lens_configuration instead
| <EuiButtonIcon | ||
| data-test-subj="observabilityAiAssistantNewChatButtonButton" | ||
| iconType={EuiIconNewChat} | ||
| iconType="newChat" |
There was a problem hiding this comment.
newChat icon is added to Eui and Eui upgrade is included in Kibana so we can get rid of our own implementation.
|
@elasticmachine merge upstream |
| }; | ||
| onUpdate(newAttributes); | ||
| }; | ||
| core.theme.theme$.subscribe(async (theme) => { |
There was a problem hiding this comment.
Probably want to avoid async here - if the getEditLensConfiguration call fails, there's no error handling (subscribe doesn't care what you return from the function)
| core.theme.theme$.subscribe(async (theme) => { | ||
| const Component = await getEditLensConfiguration(core, deps, visualizationMap, datasourceMap); | ||
| const ConfigPanel = ( | ||
| <EuiThemeProvider colorMode={theme.darkMode ? 'dark' : 'light'}> |
There was a problem hiding this comment.
Is this for Lens to fix, or for us to fix?
There was a problem hiding this comment.
Ultimately it is for Lens to fix this as we don't own the code. This comment is aimed at the Lens team (or anyone feeling qualified to answer)
There was a problem hiding this comment.
Stratoula already mentioned this I think, what I mean is that we're not wrapping wherever we mount the Lens component in a EuiThemeProvider
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we mount the edit lens component manually though.
There was a problem hiding this comment.
We do and possibly this can be fixed on the assistant too. But I am fine if the fix is applied to the lens side too. 👍
stratoula
left a comment
There was a problem hiding this comment.
looks great! Thanx Coen ❤️
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
This aims to correct dark mode rendering in both flyout and Conversation App.
Before:
Flyout - Dark Mode:
Conversation UI - Dark Mode
After:
Flyout - Dark Mode
Flyout - Light Mode
Conversation UI - Dark Mode
Conversation UI - Light Mode
