Skip to content
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

Series color improvements #70161

Open
smith opened this issue Jun 28, 2020 · 5 comments
Open

Series color improvements #70161

smith opened this issue Jun 28, 2020 · 5 comments
Labels
Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture

Comments

@smith
Copy link
Contributor

smith commented Jun 28, 2020

At this time there are some places where we're applying colors to series on the server using getVizColorForIndex:

  • Metrics charts
  • GC charts
  • Transaction breakdown chart

Since these colors are populated on the server there's no way to detect or apply a theme (dark/light.) This isn't a problem at this time since the colors we're using (euiColorVis0-9) aren't theme dependent and are the same whether you're on light or dark mode. However, if we were to change these colors we wouldn't be able to apply themes to them.

Calculating the colors on the client is probably a better idea anyway since these colors are part of the presentation and not part of the actual data.

Also, we can use euiPaletteColorBlind instead our existing list of colors. It's very similar but provided by EUI and following their recommendations. Since EUI does not currently work on the server we're unable to take advantage of this and other EUI methods where we generate colors and can only use the theme JSON imports.

@smith smith added Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture labels Jun 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@sorenlouv
Copy link
Member

sorenlouv commented Aug 31, 2020

From elastic/eui#2074 it sounds like EUI does not support SSR - but I don't think that's a requirement for us. We are just interested in detecting the selected theme (dark or light) and then use the appropriate variables.

I think it is possible to load the variables files server-side (it's just JSON, so why not?):

import euiDarkVars from '@elastic/eui/dist/eui_theme_dark.json';
import euiLightVars from '@elastic/eui/dist/eui_theme_light.json';

If that's the case we just need to determine the selected theme. Something I hope we can do if we have access to the request. If that's possible we can make a helper like this

const theme = getTheme(request)

@smith
Copy link
Contributor Author

smith commented Aug 31, 2020

I think the colors should be determined on the client-side only and sending the theme in the request and determining it there is unnecessary complexity and adds too much coupling between the data and presentation.

@sorenlouv
Copy link
Member

sending the theme in the request and determining it there is unnecessary complexity and adds too much coupling between the data and presentation

I didn't mean to send the theme in the request from the client. Rather, I expect that we can retrieve the theme on the server if we have access to the request.

Regarding the coupling of data and presentation I'm not sure if it's a problem. I agree, that a public API aimed at a diverse set of consumers probably shouldn't have anything view specific. But our endpoints are already very view-specific. They are tailormade to provide the specific bits of data that the ui needs.
I've been thinking about merging some of the endpoints, so the browser only has to make 1 api request instead of 4 API requests to display the transactions details page (for instance). If we go down that route I think it would actually reduce complexity if we move some things to the server, so we do all the transformations once on the server, instead of both on the server and afterwards on the client.

@ifrimere
Copy link

+1

offering some theme based options could be interesting beyond the traditional light/dark modes; maybe earth tones vs. pastels type options? For cloud ES, being able to set a default theme to colors that our end users could override in their viewing instances would be nice to have too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

4 participants