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

Color sync configurable on dashboard #81976

Closed
flash1293 opened this issue Oct 29, 2020 · 10 comments · Fixed by #86180
Closed

Color sync configurable on dashboard #81976

flash1293 opened this issue Oct 29, 2020 · 10 comments · Fixed by #86180
Assignees
Labels
enhancement New value added to drive a business result Feature:Dashboard Dashboard related features Feature:Lens Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure

Comments

@flash1293
Copy link
Contributor

flash1293 commented Oct 29, 2020

Currently Lens visualizations don't sync colors for same series across charts while Vislib based charts do so using the color service of the charts plugin.

To give the user more control of the inherent trade-offs of per-chart/per-dashboard color assignment, there should be a setting on dashboards to turn color sync on/off for both Lens and Visualize based charts.

Behavior

If "Sync colors across charts" is active, all charts on the dashboard using the same palette will make sure to assign the same color to the same series name even across charts. If it's turned off, each chart separately uses the palette based on the position of the series in the chart.

Screenshot 2020-10-29 at 10 08 04

Rollout

Currently there's a mixture of both behaviors - Visualize charts always use the color syncing behavior while Lens charts never use it. Once the setting is introduced, all dashboards default to "on" - this means Lens charts on existing dashboards will pick up the color sync behavior. This is a breaking change and has to be documented as such. The mixture mode won't be available anymore. Both existing dashboards and newly created dashboards will default to "on".

This behavior is closest to the currently released state - in 8.0 we might change the default setting from "on" to "off" for newly created dashboards because it's a more common use case.

Tentative phasing:

  • 7.11 roll out palettes service with current behavior (vislib palette is synced, default palette is not)
  • (probably 7.12) Once both Visualize xy and pie charts are migrated to elastic-charts and palette service, introduce setting like described below
  • 8.0 Maybe switch default from syncing to not syncing

Implementation

The setting from a dashboard has to be passed through several layers:

Dashboard

The dashboard application controls the setting - sync colors is a flag in the dashboard state similar to "Use margins between panels" and "Show panel titles". To give more flexibility in migrating the state later, it should have an explicit undefined state as default (that way we can differentiate an explicit set vs. an untouched dashboard when doing breaking changes later).

Embeddable container

The dashboard app passes the flag to the embeddable container rendering the actual panels. The container passes the flag to all embeddables via embeddable input. To do so, the default EmbeddableInput interface has to be extended. All embeddables should handle it accordingly (in some cases like saved searches this just means ignoring it).

Embeddables (Visualize and Lens)

Embeddables actually rendering charts using the palette service read the flag from the provided input and pass them to the underlying expression renderer. To do so, both loader and the expression renderer component have to be extended to take an additional property for this

Expression infrastructure

The expression infrastructure passes the flag to the expression renderer as a new property on IInterpreterRenderHandlers (similar to the "mode": #77161)

Expression renderer (Lens xy and pie renderer, Visualize xy and pie renderer)

All expression renderers internally using the palette service are responsible for fetching the flag from the provided render handlers and passing it to the actual call to the palette instance getColor method.

Palette service

The getColor method has to be extended by a new parameter as well which takes the color flag provided by the expression renderer. All individual palette implementations are responsible for using the flag making sure the same series will be color in the same way. Some palettes (e.g. gradient based palettes) can opt to simply ignore the flag. To implement the color sync, the MappedColors class can be used (currently used by the legacy vislib palette implementation as well). If the flag is set to false, both the default palette and the legacy vislib palette should fall back to pick a color based on the position of the current series in the current chart.

@flash1293 flash1293 added Feature:Dashboard Dashboard related features enhancement New value added to drive a business result Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:AppArch Feature:Lens labels Oct 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@timductive
Copy link
Member

@flash1293 I might be confused but what would be the use-case for "not syncing" the colors across charts? I would read this as almost a bug in Lens, that it doesn't automatically sync across charts, especially if vislib charts do.

I don't understand why we would default to not syncing in 8.0.

@AlonaNadler @clintandrewhall

@flash1293
Copy link
Contributor Author

flash1293 commented Oct 29, 2020

@timductive Great question, I thought the same initially. I can't put it in words as eloquently as @markov00 , but in general less colors are better - if colors are synced across charts, you need much more of them because now you have to make sure there are no collisions among all series on the dashboard instead of just those of a single chart. In a lot of cases the inter-chart consistency doesn't matter, and this provides a nice way to turn it off and have the nice looking colors at the beginning of the palette in all charts (further out in the future we might also allow more fine-grained control about which things to sync, e.g. just the series based on a certain field out of a certain index patterns, or just series in two specific charts). The reason not to make this the default right away is that it's much closer to the current behavior to have color sync enabled.

That being said - that's just a maybe based on todays perspective. Once we get closer to 8.0, things might have changed.

@timductive
Copy link
Member

Thanks @flash1293 that makes more sense. My default reaction would still be, can we tell what an average number of series per dashboard would be? This would tell us if the switch should default to on or off.

I'm assuming we don't have this data. It might be a good thought exercise for @crob611 @ThomThomson as we work on the new telemetry service.

@flash1293
Copy link
Contributor Author

@timductive Great idea to base this decision on data - until 8.0 we have enough time to do so.

@AlonaNadler
Copy link

We had this capability in Visualize, most users don't realize it sincethis capability works in specific charts and only if users have the same values in these charts

image
As you can see the entire dashboard visualize 5 teams and it is not clear there is a color sync (this dashboard is almost the ideal dashboard to demonstrate this capability)

There are a few reasons to want this capability:

  • it helps users correlate the same values
  • it makes the dashboard looks more consistent and aesthetic

backing that with data will be hard, mainly because even if you see how many series dashboards have these are not necessarily the same values and will have different colors even with this capability

Talking with @markov00 and @flash1293 I'm convinced the decision we took is the right one

@timductive
Copy link
Member

Thanks @AlonaNadler that makes sense.

@timroes
Copy link
Contributor

timroes commented Nov 24, 2020

For posterity: We synced earlier about this and decided that the new colors in the classical xyaxis and pie chart (which are currently converted to elastic-charts) will only be switched over to the new color palette by default as soon as we build the sync mode on dashboards, so we're not introducing too many color changes in consecutive minor versions.

Related:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Feature:Dashboard Dashboard related features Feature:Lens Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:Visualizations Visualization editors, elastic-charts and infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants