-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Fix discover, tsvb and Lens chart theming issues #69695
Conversation
Pinging @elastic/kibana-app (Team:KibanaApp) |
@@ -0,0 +1,92 @@ | |||
# Theme Service | |||
|
|||
The `theme` service offers utilities to interact with the kibana theme. EUI provides a light and dark theme object to suplement the Elastic-Charts `baseTheme`. However, every instance of a Chart would need to pass down this the correctly EUI theme depending on Kibana's light or dark mode. There are several ways you can use the `theme` service to get the correct shared `theme` and `baseTheme`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `theme` service offers utilities to interact with the kibana theme. EUI provides a light and dark theme object to suplement the Elastic-Charts `baseTheme`. However, every instance of a Chart would need to pass down this the correctly EUI theme depending on Kibana's light or dark mode. There are several ways you can use the `theme` service to get the correct shared `theme` and `baseTheme`. | |
The `theme` service offers utilities to interact with the kibana theme. EUI provides a light and dark theme object to supplement the Elastic-Charts `baseTheme`. However, every instance of a Chart would need to pass down the correct EUI theme depending on Kibana's light or dark mode. There are several ways you can use the `theme` service to get the correct shared `theme` and `baseTheme`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gracias 189843c
|
||
## Why have `theme` and `baseTheme`? | ||
|
||
The `theme` prop is a recursive partial `Theme` that overrides properties from the `baseTheme`. This allows changes to the `Theme` type in `@elastic/charts` without having to update the `@elastic/eui` themes or every `<Chart />`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The `theme` prop is a recursive partial `Theme` that overrides properties from the `baseTheme`. This allows changes to the `Theme` type in `@elastic/charts` without having to update the `@elastic/eui` themes or every `<Chart />`. | |
The `theme` prop is a recursive partial `Theme` that overrides properties from the `baseTheme`. This allows changes to the `Theme` TS type in `@elastic/charts` without having to update the `@elastic/eui` themes for every `<Chart />`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that every <Chart />
instance now must provide both the baseTheme
and the EUI provided theme
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a must. But since we added the background color to the theme for the contrast ratio logic, the background color defaults to a non-transparent
colors respective of the mode (i.e. light and dark). So in order for the chart to work in both dark and light modes without a baseTheme
, one would need to set the theme.background.color
to 'transparent'
. This would avoid the contrast logic though.
I'm not sure if 'transparent'
should just be the default for the background color or if you just want to add this to the EUI themes.
But on a more general note, I think it's helpful to have a baseTheme
used in all elastic/charts Charts
in kibana for times when we change the Theme
object. This allows for nothing to break while in the meantime we update any changes to the EUI charts partial themes.
I raised the question in an EC issue, to see how we could do this better (see elastic/elastic-charts#718). As of now, I think a HOC to handle this logic and allow overrides would be the most practicable option. Something like...
const MyComponent = () => (
<Chart>
<ThemedSettings theme={EuiThemeOverrides} {...otherSettingsProps} />
{/* more goodness */}
</Chart>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed spelling errors here 189843c
}, | ||
}, | ||
]} | ||
baseTheme={baseTheme} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to me like it no longer is getting the EUI theme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think TSVB was ever using the EUI charts theme. The original getTheme
code below only depended on the @elastic/charts
themes. This is pretty clear from the UI of TSVB compared to charts using EUI, EUI themed charts look much nicer! 😝
Lines 97 to 131 in e3d01bf
export function getTheme(darkMode: boolean, bgColor?: string | null): Theme { | |
if (!isValidColor(bgColor)) { | |
return darkMode ? DARK_THEME : LIGHT_THEME; | |
} | |
const bgLuminosity = computeRelativeLuminosity(bgColor); | |
const mainTheme = bgLuminosity <= 0.179 ? DARK_THEME : LIGHT_THEME; | |
const color = findBestContrastColor( | |
bgColor, | |
LIGHT_THEME.axes.axisTitleStyle.fill, | |
DARK_THEME.axes.axisTitleStyle.fill | |
); | |
return { | |
...mainTheme, | |
axes: { | |
...mainTheme.axes, | |
axisTitleStyle: { | |
...mainTheme.axes.axisTitleStyle, | |
fill: color, | |
}, | |
tickLabelStyle: { | |
...mainTheme.axes.tickLabelStyle, | |
fill: color, | |
}, | |
axisLineStyle: { | |
...mainTheme.axes.axisLineStyle, | |
stroke: color, | |
}, | |
tickLineStyle: { | |
...mainTheme.axes.tickLineStyle, | |
stroke: color, | |
}, | |
}, | |
}; | |
} |
That said I think we could make TSVB use the EUI theme in another PR. It should be easy to merge the getTheme
logic with the EUI themes.
import { getTheme } from './theme'; | ||
import { getBaseTheme } from './theme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you trying to completely replace getTheme
with getBaseTheme
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by replace it?
This was just a utility function to get the theme which was actually generating a base theme so I just updated the naming.
Ideally, there is a baseTheme
from @elastic/charts
. Then this function returns a Partial<Theme>
using EUI that passes the value to the Chart.theme
prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to apply the proposed changes to fix crashing of our component which using Chart
...ers_actions_ui/public/application/components/builtin_alert_types/threshold/visualization.tsx
Show resolved
Hide resolved
...ers_actions_ui/public/application/components/builtin_alert_types/threshold/visualization.tsx
Outdated
Show resolved
Hide resolved
...ers_actions_ui/public/application/components/builtin_alert_types/threshold/visualization.tsx
Outdated
Show resolved
Hide resolved
...ers_actions_ui/public/application/components/builtin_alert_types/threshold/visualization.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changes, alerting code LGTM
@@ -26,72 +26,4 @@ Truncated color mappings in `value`/`text` form | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's irrelevant with this PR but I saw on L21 that there is a typo Funciton instead of Function and retrive instead of retrieve. Do you want to also correct it in this PR ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d82c6b8
@@ -56,7 +56,6 @@ const handleCursorUpdate = (cursor) => { | |||
}; | |||
|
|||
export const TimeSeries = ({ | |||
darkMode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also remove the darkMode prop Type on L276
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the darkMode
as it was only used to pass to the getTheme
function to select the baseTheme
from @elastic/charts
which is what the useChartBaseTheme
hook does on it's own.
Sorry misread that, yes I'll remove the propType, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in d82c6b8
@nickofthyme I noticed that while now the bg color change works as it should there is a problem when applying color with transparency. For example: As you can see the border has the correct color but the main container is darker. On 7.8 instance the background color appears smoothly |
@stratoula great find on the transparency. It looks like tsvb was applying the bgColor to the parent div and adding padding. Since there is currently no way to padding around the entire chart via the See d82c6b8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I got really confused as to what the differences in chart theme services there are. There's a theme provider from Charts and one from EUI? Ack I dunno, super confused but so long as no one else is... 🤷
padding: $euiSizeS; | ||
border-width: $euiSizeS; | ||
border-style: solid; | ||
border-color: transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look at this and think "Why not padding?" Can you please add a comment here as to why it's necessary to do it this way so no one tries to optimize by converting it back to padding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a workaround for this issue.
Bascially, the containing element for TSVB was applying a background color and padding. This was fine when elastic charts background was transparent but not anymore.
I think this can be cleaned up when we allow padding around the whole chart in the Theme
but for now changing the theme.chartMargins
does not include the legend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no worries. Just suggesting adding a comment in the SASS file to keep others aware if they jump in this file later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cchaos The basic idea is each chart should have a baseTheme (for light and dark) and an EUI theme (for light and dark). The EUI theme is only overriding some of the theme values. Without setting the baseTheme we automatically use our I can jump on a zoom call to explain more if you'd like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nickofthyme, that makes sense to me. However, if EUI isn't changing some colors from the base theme it's probably not going to align with EUI colors. Can you send me the new theme keys that the EUI theme isn't addressing?
padding: $euiSizeS; | ||
border-width: $euiSizeS; | ||
border-style: solid; | ||
border-color: transparent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no worries. Just suggesting adding a comment in the SASS file to keep others aware if they jump in this file later.
@flash1293 can you check on fb35a49 when you get a chance? BTW the |
@nickofthyme Seems like some unit tests have to be adjusted. Let me know when I can help here. |
- fix broken jest tests in lens - update datapannel to use charts theme - update FieldsAccordion to use charts theme
Thanks @flash1293 I updated tests and fixed the type errors |
@flash1293 or @wylieconlon I made some changes to Lens to use the charts plugin to grab the correct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for fixing this in Lens as well, @nickofthyme - really appreciate the help.
I only have one nit (but If you want to merge this PR before fixing it, I'm fine with it as well)
In the pie chart, the outer labels don't have great contrast in dark mode:
This PR:
@flash1293 Thanks for taking a look. Yeah this PR was supposed to enhance the color contrast for partition charts, however we ran into an issue where to do so we would need to set the background color on all charts in kibana. For now, the background color is set to I added the background color to the EUI charts theme here so the next version should automatically give you the ability to enhance the text contrast on the partition charts using a config like below. See elastic/elastic-charts#608 config: {
fillLabel: {
textInvertible: true,
textContrast: true, // can also be set to a number
}
} |
* master: Update known-plugins.asciidoc (elastic#69370) [ML] Anomaly Explorer swim lane pagination (elastic#70063) [Ingest Manager] Update asset paths to use _ instead of - (elastic#70320) Fix discover, tsvb and Lens chart theming issues (elastic#69695) Allow Saved Object type mappings to set a field's `doc_values` property (elastic#70433) [S&R] Support data streams (elastic#68078) [Maps] Add styling and tooltip support to mapbox mvt vector tile sources (elastic#64488) redirect to default app if hash can not be forwarded (elastic#70417) [APM] Don't fetch dynamic index pattern in setupRequest (elastic#70308) [Security_Solution][Endpoint] Leveraging msearch and ancestry array for resolver (elastic#70134) Update docs for api authentication usage (elastic#66819) chore(NA): disable alerts_detection_rules cypress suites (elastic#70577) add getVisibleTypes API to SO type registry (elastic#70559)
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/index_management/home_page·ts.Index Management app Home page Component templates renders the component templates tabStandard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_management/templates·js.apis management index management index templates get all should list all the index templates with the expected parametersStandard Out
Stack Trace
Kibana Pipeline / kibana-xpack-agent / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_management/templates·js.apis management index management index templates get all should list all the index templates with the expected parametersStandard Out
Stack Trace
and 1 more failures, only showing the first 3. Build metrics
History
To update your PR or re-run it, just comment with: |
Summary
Fixes #69667 and #70181
baseTheme
from charts plugin theme service.theme
service in theCharts
plugin to include base theme.Adds the following to theme service:
chartsDefaultBaseTheme
darkModeEnabled$
- observable for dark mode booleanchartsBaseTheme$
- observable to get base theme based on dark modeuseChartsBaseTheme
- React hook to get base theme based on dark modeScreenshots
TSVB
Discover
Checklist