-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Uptime] Use EUI color palette for charts/histograms #29439
[Uptime] Use EUI color palette for charts/histograms #29439
Conversation
💚 Build Succeeded |
@justinkambic I'd suggest consuming the JSON variable files that exist in Some docs for this are in EUI's consuming.md docs. You can also see a working example in React in use by the Infra UI team over here. I don't recommend using styled components, but you can at least see a good example of theme switching there. I'll add something more concrete in the docs in EUI's guidelines to show this stuff off now that Dark Theme is a real thing. |
@snide that's the kind of direction I was hoping for, thank you! |
fa962f1
to
8ce3bfb
Compare
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
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.
Looks good. Thanks for the changes.
@@ -25,14 +31,14 @@ export const SnapshotHistogram = ({ histogram }: SnapshotHistogramProps) => { | |||
name={i18n.translate('xpack.uptime.snapshotHistogram.series.upLabel', { | |||
defaultMessage: 'Up', | |||
})} | |||
color="green" | |||
color={primaryColor} |
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.
To be safe I always recommend pulling from the EuiVisColor
series for anything chart related because they're targeted towards color blindess in charts. https://elastic.github.io/eui/#/guidelines/colors
The rest of this looks solid.
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.
@snide the primaryColor
value in this case is either euiDarkVars.euiColorVis1
or euiLightVars.euiColorVis1
- is that not an appropriate value to use?
https://github.com/elastic/kibana/pull/29439/files#diff-abd19a861ef67d295beeea16e5a44b0bR120
https://github.com/elastic/kibana/pull/29439/files#diff-abd19a861ef67d295beeea16e5a44b0bR126
jenkins test this |
💚 Build Succeeded |
💚 Build Succeeded |
* Add hardcoded eui primary/danger to charts/histograms. * Use EUI JSON vars for color values.
Summary
Previously the charts/histograms in the Uptime app were using hardcoded default CSS colors
red
andgreen
. We should at least adhere to the EUI color palette.Update:
Now using EUI JSON variables per @snide's suggestion:
Old
Further direction
Please recommend a better way of doing this if it doesn't adhere to best practices. EUI expectations for this sort of thing is a little obscure to me at the moment and I'm happy to take direction.
Testing this PR
You should be able to load the charts on
Overview
/Monitor
pages and see the new colors