[WIP] ml fixes for dark mode #33222
[WIP] ml fixes for dark mode #33222
Conversation
| import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json'; | ||
| import euiLightVars from '@elastic/eui/dist/eui_theme_light.json'; | ||
|
|
||
| export const euiColorForTheme = (euiColor) => { |
There was a problem hiding this comment.
This function seemed nice and generic. I had no idea where it should live so I put in ML for now so I could reuse it. Any ideas @cchaos?
There was a problem hiding this comment.
We can ask the platform team if they know a good spot to put this a service anyone can grab. But I'd name it more specifically too like getEuiThemeColorsForJS
There was a problem hiding this comment.
Oh I see, you pass back the the actual color value, not just the object.... Maybe even make it more generic then like euiColor
💔 Build Failed |
|
Hey Dave! I like the idea that the chart's line and model plot area have a different color than the low severity anomalies. The chart's line and model plot are a "generic" color while the low severity anomalies are historically being blue as part of that severity color range. Just one nit pick thought: The brush element (the border and handles with arrows) to select the time range in the lower context chart is an interaction element so I think it would be better if it was the same blue as the buttons and links, what do you think? |
|
I'll give it a try!
…On Thu, Mar 14, 2019, 2:30 AM Walter Rafelsberger ***@***.***> wrote:
Hey Dave! I like the idea that the chart's line and model plot area have a
different color than the low severity anomalies. The chart's line and model
plot are a "generic" color while the low severity anomalies are
historically being blue as part of that severity color range. Just one nit
pick thought: The brush element (the border and handles with arrows) to
select the time range in the lower context chart is an interaction element
so I think it would be better if it was the same blue as the buttons and
links, what do you think?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#33222 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AATzp03BY0JATXxh2xa2xNN_nI30X2cuks5vWhbBgaJpZM4bzVFm>
.
|
| import chrome from '../../../../../src/legacy/ui/public/chrome'; | ||
| const IS_DARK_THEME = chrome.getUiSettingsClient().get('theme:darkMode'); | ||
| import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json'; | ||
| import euiLightVars from '@elastic/eui/dist/eui_theme_light.json'; |
There was a problem hiding this comment.
I'm not going to worry about it now, but this an instance where we'll run into trouble when we start having custom theming.
There was a problem hiding this comment.
Yes. There will eventually be a theme provider. But at least if we use this it'll all be in one place so we just update the one file. That's why I like euiColorForTheme as a name? Guess it doesn't matter too much.
There was a problem hiding this comment.
Yeah I was just thinking on the consumer side, they don't need to care about the theme part of it, they just care that they're getting the right color. So something simple like euiColor would be easier for them to remember. Though, maybe it shouldn't be prefixed with eui since it could be a custom theme. So maybe just getColor though that's probably too generic... dunno maybe more of a kibana service so kbnGetColor?
💔 Build Failed |
💔 Build Failed |
|
Heya @snide - thanks for your work on this! 😄 Just a couple of comments: IMO keeping the severity colors - light blue, blue, yellow, orange, and red - consistent is rather important. Here, the light blue becomes gray.
The annotations are a bit hard to see - maybe more contrast? |
|
@snide on the whole these changes look great. Echoing what @alvarezmelissa87 says, I think some of the anomaly severity colors need some tweaks. In the charts in the Single Metric Viewer and the Anomaly Explorer, the orange (scores 50-75) now looks more like a red, whilst the red color (scores 75-100) has become pink. Can we stick with the light blue, blue, yellow, orange, red colors, even if you think the exact shades need tweaking (e.g. for better accessibility contrast)? Does the area outside the slider have enough contrast? Before it had a light gray color from the
Your edits to the background color of the event rate chart in the multi/population wizard has shown up that the
Do you think the font color for text on these cards in the the Data Visualizer has enough contrast? ('Calculated over all documents') This filter bar is new for 7.1 - looks like we need to make some changes for dark theme here: |
Summary
Fixes #30368
This is a WIP PR that touched a bunch of spots in ML. Prolly needs one more day of deeper testing, but if anyone from @elastic/ml-ui wants to give this a quick spin to spot things I missed give it a go. I changed some colors around to make them a little more K7 style, hopefully they fit with the mood. I made a util for picking euiColors based on theme (which should likely be someplace more global, will chat with @cchaos about that later), but also defined the ML vars in one spot so we can pull them in as needed. My guess is that there's lots more places in the JS that are using these that I should replace, but this is a good start and addresses most of the major issues.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers