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

feat: improve color consistency #19038

Merged
merged 51 commits into from
Mar 21, 2022

Conversation

stephenLYZ
Copy link
Member

SUMMARY

The difference between this PR and this one is that all label colors of the dashboard are saved to handle the native filter scenario.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

before

2022-03-06.11.02.05.mov

after

2022-03-06.11.00.34.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@stephenLYZ
Copy link
Member Author

@geido Nice catch! Fixed here.
image

@geido
Copy link
Member

geido commented Mar 18, 2022

/testenv up

@github-actions
Copy link
Contributor

@geido Ephemeral environment spinning up at http://50.112.38.121:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido
Copy link
Member

geido commented Mar 18, 2022

@stephenLYZ I am worried that something happened to the ability to bring the colors of a chart from its Dashboard scheme to Explore. As you can see the colors in Explore look different from the Dashboard.

untitled.dashboard.mp4

@stephenLYZ
Copy link
Member Author

stephenLYZ commented Mar 19, 2022

@geido Yes, it's a known issue and it only exists in the test environment, see Michael's comment. But in local env it cannot be reproduced.

@stephenLYZ I think I know what's the problem here. This #18976 changed the default cache type to NullCache when not in debug mode. I think this is affecting the test environments. @villebro do you know how can we change this config for test environments?

@zhaoyongjie zhaoyongjie self-requested a review March 21, 2022 07:08
Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Tested in local. work as expected.

@zhaoyongjie zhaoyongjie merged commit dc57508 into apache:master Mar 21, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

michael-hoffman-26 pushed a commit to nielsen-oss/superset that referenced this pull request Mar 23, 2022
villebro pushed a commit that referenced this pull request Apr 3, 2022
@serenajiang
Copy link
Contributor

Hi, we've had some reports of unexpected colors in larger dashboards. From looking at this PR, I think this is because new colors are autogenerated once you "run out" of colors in the color scheme (correct me if I'm wrong?). I understand this is to prevent the same color being used twice, but non-unique dashboard colors are often not necessary if there are no conflicts within a single chart.

I'm instructing users to override colors with label_colors if they don't like the autogenerated "similar to color scheme" ones, but this is a pretty big hassle, and it's a bit confusing because we don't surface shared_label_colors in the UI.

Thoughts?
@zhaoyongjie @stephenLYZ

@clausherther
Copy link

What's particularly disruptive about this change is that it seems to cause colors to get auto-picked that are outside the defined color scheme. It also seems unclear what causes one chart to get a color auto-assigned vs another that has a color from the palette.

Hi, we've had some reports of unexpected colors in larger dashboards. From looking at this PR, I think this is because new colors are autogenerated once you "run out" of colors in the color scheme (correct me if I'm wrong?). I understand this is to prevent the same color being used twice, but non-unique dashboard colors are often not necessary if there are no conflicts within a single chart.

I'm instructing users to override colors with label_colors if they don't like the autogenerated "similar to color scheme" ones, but this is a pretty big hassle, and it's a bit confusing because we don't surface shared_label_colors in the UI.

Thoughts? @zhaoyongjie @stephenLYZ

@stephenLYZ
Copy link
Member Author

stephenLYZ commented Apr 21, 2022

@serenajiang Hi, thanks for the feedback. This PR is not only to prevent color conflicts caused by "run out" of colors in the color-scheme, but mainly to ensure that the same dimensions of all charts in the dashboard have the same color.

The shared_label_colors is transparent to the user. It stores the colorMap({[chartId]: { [label]: color }}) of all charts in the dashboard, and if different charts have the same dimensions, these dimensions will be extracted to generate analogous colors of color scheme, which is to prevent color conflicts.

The question is why we need to extract these same dimensions and use analogous colors? In this case, if we use the remaining color of color-scheme for the same dimensions in the one chart, we can't be sure whether the remaining color has been used in another chart, which may lead to color conflicts in another chart.

For example, one dashboard has chartA and chartB. The color scheme is ['red', 'yellow', 'blue']. The chartA has two dimensions ['a', 'b'] and chartB has three dimensions ['b', 'c', 'e'].
As before the color map is
chart A

{
  a: 'red',
  b: 'yellow',
}

chart B

{
  b: 'red',
  c: 'yellow',
  e: 'blue',
}

But we need to make sure that dimension b has the same color in the two charts. If we both use yellow as the color value for dimension b, it will cause a color conflict for chartB. That's the problem I want to solve.

@clausherther
Copy link

Hi @stephenLYZ, the use case you're describing makes sense - keeping chart dimension colors consistent. This was already possible via label_colors. The issue we're running into is that on a dashboard with a number of different charts, new charts suddenly get assigned random colors outside of Airbnb's color palette. This is unexpected and undesirable for our users and will ultimately hurt adoption of our dashboards.
What we're observing is also that charts that do not have dimensional cuts (i.e. simple single metric time-series charts without group-bys or multiple series), which in the past have all been assigned the first value in the color palette, now get assigned a new, non-palette, color value, despite having no dimensional cuts.
Would it be possible to make the behavior you've implemented optional at the dashboard level? E.g. "Cycle through dashboard theme colors: Yes/No" with "No" describing the behavior you've implemented?

@stephenLYZ stephenLYZ changed the title feat: improve color consistency (save all labels) feat: improve color consistency Apr 21, 2022
@serenajiang
Copy link
Contributor

Thanks @stephenLYZ for the thorough explanation, I see without generating colors it'd be hard to totally avoid these conflicts, and trying to fine-tune it might get really messy. However, for our deployment, I'm not sure whether we need to optimize that hard, since users can always manually override label colors. Airbnb in particular has strong loyalty to our color scheme, so unexpected colors can be a bit jarring. Making this behavior optional for each dashboard could help mitigate disruptions, or even putting it all behind a feature flag (is there a feature flag for this already?).

Re:

The shared_label_colors is transparent to the user.

Would you mind pointing me to how to view this in the UI? I couldn't view it in the json metadata when I tried to edit a dashboard, and only found it because it was in the API response.

@rusackas
Copy link
Member

Heya @clausherther and @serenajiang - thanks for raising your concerns. This is an intricate problem, but I'm confident we can find a sensible way forward so everyone gets what they want here. There are pros and cons to pretty much any approach, which I'll attempt to lay out here.

As a bit of a side note, we sometimes find label_colors to be an imperfect solution since (a) drill-down/up/across are coming, and across-the-board consistency will be of increasing importance, (b) we saw cases where people make a particular series a specific color from the palette, and then a neighboring line or pie slice was also assigned to be that color, and (c) as a feature, it's a little buried and cumbersome in general for wide adoption.

So... i think I see a couple paths forward, with varying levels of difficulty. Here goes...

  1. Add an app configuration (or dare I say Feature Flag?) for USE_ANALAGOUS_COLORS_IN_DASHBOARDS which, when enabled, would act as it does today. If you turn it off, it would simply loop through the palette colors repeatedly. Default of that config is up for debate.
  • True would mean it works like it does today, generating colors as needed:
    • Upsides:
      • Unique color per-series, meaning there will be fewer issues disambiguating charts
      • Features like cross-filters (added support for this nascent feature coming soon) and drill-down/up/across will be visually coherent
    • Downsides:
      • Depending on size of dashboard and number of series in it, there may be a proliferation of colors
  • False would mean the dashbpard cycles repeatedly through the specified color palette.
    • Upsides:
      • Only official pallete colors get used
    • Downsides:
      • You may see the same color multiple times in a chart (depending on the number of series in the chart vs the number of colors in the palette
      • A color may be assigned (via label_coors or naturally) the identical color as its neighbor, causing confusion
      • A given series in a dashbard will have a consistent color, but not a unique color, so it may be ambiguous or misleading in some use cases
  1. This is a less appealing idea since it has many potential issues, but we have heard of use cases for it, so here goes... we could add another configuration option for CONSISTENT_SERIES_COLORS_IN_DASHBOARDS which defaults to True to support either of the above treatments. If set to False however, the dashboard would ignore chart-to-chart consistency entirely, and just start at the beginning of the color palette in each chart. Then the first color in the palette (sometimes a brand color) is always the first color selected for a chart, and thus the dominant color in many dashboards.

Hope this helps, and hope we can steer this so that everyone wins :D

@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 need:qa-review Requires QA review size/XL 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants