fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

geido · 2024-03-08T15:15:57Z

SUMMARY

After the changes in this PR #23762 color inconsistency on refreshes was reintroduced.

Fixes #16970
Fixes #26456
Fixes #28104
Fixes #25384 (specifically for metrics changing colors on refreshes. An additional fix is required for charts emitting only COUNT(*) as label and not specific ones)

This PR does the following:

Fixes color inconsistency on refreshes
Fixes color inconsistency from chart in hidden tab to Explore
It helps avoiding color collisions by adding a usage tracker of the colors at the chart level
Deprecates AVOID_COLORS_COLLISION feature flag
Renames the shared labels color map to LabelColorsMap
Centralizes the logics of generating the colors map and color metadata

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

Set a color scheme for a dashboard
Refresh the dashboard
Colors should not change
Going from any chart in any tab to Explore, should carry the same colors forward

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

…o/ch79154/fix-color-inconsistency

codecov · 2024-03-08T15:29:24Z

Codecov Report

Attention: Patch coverage is 78.66667% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 67.34%. Comparing base (f453d5d) to head (36795f2).
Report is 22 commits behind head on master.

Files	Patch %	Lines
...d/src/dashboard/components/gridComponents/Tabs.jsx	9.09%	10 Missing ⚠️
...rc/explore/components/ExploreChartHeader/index.jsx	60.00%	3 Missing and 1 partial ⚠️
...frontend/src/dashboard/components/Header/index.jsx	50.00%	0 Missing and 1 partial ⚠️
...src/dashboard/components/PropertiesModal/index.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27439      +/-   ##
==========================================
- Coverage   69.61%   67.34%   -2.28%     
==========================================
  Files        1908     1909       +1     
  Lines       74543    74649     +106     
  Branches     8316     8330      +14     
==========================================
- Hits        51895    50272    -1623     
- Misses      20595    22321    +1726     
- Partials     2053     2056       +3

Flag	Coverage Δ
javascript	`57.20% <78.66%> (-0.06%)`	⬇️
mysql	`78.00% <ø> (-0.01%)`	⬇️
postgres	`78.10% <ø> (-0.01%)`	⬇️
python	`78.23% <ø> (-4.65%)`	⬇️
sqlite	`77.61% <ø> (-0.01%)`	⬇️
unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rusackas · 2024-03-08T15:46:52Z

RESOURCES/FEATURE_FLAGS.md

@@ -84,6 +84,7 @@ These features flags currently default to True and **will be removed in a future

 [//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY"

+- AVOID_COLORS_COLLISION


We might technically need to get consensus on this. @michael-s-molina any problems getting consensus and doing this now, or should we wait until the 5.0 proposals open up?

I think waiting for 5.0 would be a good idea as it allows an orderly evaluation of proposed deprecations. We just need to add a card to the board to remind us to submit the proposal.

@michael-s-molina @rusackas this feature flag introduced a regression, so I don't think it makes sense to discuss depreciation. I am keeping it here and not deleting it for now just because we don't have a breaking window open. Thoughts?

superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts

villebro · 2024-03-08T17:11:40Z

@geido would it be possible to add a brief description of the mechanics of the changes in this PR? This would help grasp the chosen approach without having to dive directly into the diff.

geido · 2024-03-08T17:45:51Z

superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts

+   * @param currentColor
+   * @returns the least used color that is not the excluded color
+   */
+  getNextAvailableColor(currentColor: string) {


This is meant to help with color collisions. It orders the colors based on their usage count in the chart. Least used colors will be preferred. This does not guarantee full protection against color collisions, which should be implemented at the individual viz based on their shape.

geido · 2024-03-08T17:47:58Z

superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx

@@ -256,10 +264,51 @@ export class Tabs extends React.PureComponent {
    }
  };

+  // the initial color map is generated on save and catches all rendered charts


Currently the initial color map is stored in the dashboard metadata at save but fails to map colors of charts that are hidden within tabs as those are not rendered. This checks for new colors popping in the color map that are not stored in metadata yet when a tab and its charts are rendered. It then updates the metadata accordingly. The stored color map is then used by the viz in Explore to respect the colors of the dashboard context.

geido · 2024-03-08T17:54:05Z

@geido would it be possible to add a brief description of the mechanics of the changes in this PR? This would help grasp the chosen approach without having to dive directly into the diff.

@villebro this is meant to fix the regression introduced in #23762 so there is not much more context to add. However, I added inline/GitHub comments to the parts of the code that I think introduce new behaviours or that might require clarifications.

geido · 2024-03-11T10:56:01Z

A few thoughts about the current state of the implementation and the way forward:

SharedLabelColorSingleton was meant to store only those labels that are shared across multiple charts. However, that is not the case since this is used to store all label colors independently if shared or not. This should get a better name, probably LabelColorsMap and change its references everywhere.
There are multiple places that are storing the color map in the dashboard metadata, including saving the properties and clicking on tabs (as of the changes in this PR), on verifyUpdateColorScheme and others. A larger refactoring work is required to coordinate and centralize changes to the metadata. For example, we might centralize updating the metadata when individual tabs load.
There are instances in which the underlying data of a chart might change and new labels show up or get removed. Currently, the implementation does not store those in the metadata as the saving process only happens when editing properties or when switching tabs. This requires a fix but it makes sense to apply it in the lenses of a larger refactor.
CategoricalColorScale and CategoricalColorNamespace, as well as SharedLabelColorSingleton should be refactored to improve the hierarchy and lower interdependencies. This PR attempts to improve on that but more refactoring work is required.
While this PR improves issues with color collisions by introducing a color count at the CategoricalColorScale level, specific logics should be implemented at the individual viz types as order and shapes affect collisions and there is hardly a solution that can account for all possible shapes.

CC @eschutho

geido · 2024-03-11T11:08:09Z

/testenv up

github-actions · 2024-03-11T11:48:13Z

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

superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts

betodealmeida

Looks good to me, and I really like your thoughts on moving forward — this seems very complicated as is today.

kgabryje

LGTM, sorry for the long delay after my first review!

…o/ch79154/fix-color-inconsistency

mistercrunch · 2024-05-10T02:14:04Z

I'd love to see a user-oriented FAQ entry "How do colors get assigned to my dashboard?"

geido · 2024-05-15T16:01:36Z

/testenv up

github-actions · 2024-05-15T16:14:29Z

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

…o/ch79154/fix-color-inconsistency

geido · 2024-05-28T17:07:12Z

/testenv up

github-actions · 2024-05-28T17:19:18Z

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

…o/ch79154/fix-color-inconsistency

sadpandajoe · 2024-06-06T17:18:17Z

I think I may have broken a chart if giving too many data points

Repro steps:

Create a pie chart
Dimensions: name
Metric: sum(rank)
row limit: 100
color scheme: red to yellow
This will create a chart
bump the row limit to 10000
This will show white as a color which is not part of the red to yellow palette and if you hover over the white slice, there are actually multiple items in there

sadpandajoe · 2024-06-06T17:27:29Z

Color collision still happens sometimes on stacked bar charts

Steps to repro:

Create a bar chart with video games sales dataset
x-axis: platform
metric: sum(rank)
dimensions: name
row limit: 100
color theme: blue to green
stacked style: stack

notice that the GC and PS4 bar have the same color on top of each other

geido · 2024-06-20T12:37:48Z

Color collision still happens sometimes on stacked bar charts

Steps to repro:

Create a bar chart with video games sales dataset

x-axis: platform

metric: sum(rank)

dimensions: name

row limit: 100

color theme: blue to green

stacked style: stack

notice that the GC and PS4 bar have the same color on top of each other

After a deeper investigation, it appears that this issue is due to how data is grouped. This is one of those issues that require a dedicated color algorithm as the default one can only check the color usage in the order as the data is received, not in the way it is ultimately grouped and visualized.

geido · 2024-06-20T12:42:11Z

I think I may have broken a chart if giving too many data points

Repro steps:

Create a pie chart

Dimensions: name

Metric: sum(rank)

row limit: 100

color scheme: red to yellow

This will create a chart

bump the row limit to 10000

This will show white as a color which is not part of the red to yellow palette and if you hover over the white slice, there are actually multiple items in there

This seems to be a problem with the Pie chart in general as I can reproduce it on master too. I'll track it for a separate investigation.

github-actions · 2024-06-20T13:30:46Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit 313ee59)

geido added 5 commits March 1, 2024 17:21

Merge branch 'master' of https://github.com/apache/superset into dieg…

91f0d51

…o/ch79154/fix-color-inconsistency

Prefer less used colors

43fe162

Merge branch 'master' of https://github.com/apache/superset into dieg…

51bf74f

…o/ch79154/fix-color-inconsistency

Update map on hidden tabs

6f89a9c

Clean up

83cf930

geido requested review from jinghua-qa, eschutho, rusackas and betodealmeida as code owners March 8, 2024 15:15

pull-request-size bot added the size/XL label Mar 8, 2024

github-actions bot added the packages label Mar 8, 2024

geido changed the title ~~fix: Color inconsistency on refreshes and conflicts~~ fix(Dashboard): Color inconsistency on refreshes and conflicts Mar 8, 2024

rusackas reviewed Mar 8, 2024

View reviewed changes

superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts Show resolved Hide resolved

Apply same color to same label value

a3e7ae8

geido commented Mar 8, 2024

View reviewed changes

geido added 3 commits March 8, 2024 19:14

Update tests

7706426

Update e2e tests

664d799

Increase coverage

36795f2

geido requested a review from kgabryje March 14, 2024 14:07

kgabryje reviewed Mar 14, 2024

View reviewed changes

superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts Outdated Show resolved Hide resolved

betodealmeida approved these changes Mar 22, 2024

View reviewed changes

kgabryje approved these changes Mar 22, 2024

View reviewed changes

geido added 2 commits May 7, 2024 15:27

Merge branch 'master' of https://github.com/apache/superset into dieg…

211bfc0

…o/ch79154/fix-color-inconsistency

[WIP] Refactor saving logics

eadc4c3

pull-request-size bot added size/XXL and removed size/XL labels May 9, 2024

geido added 4 commits May 13, 2024 16:40

Merge

2eed5e5

Clean up

c81a55f

Move utils outside dashboard

33e0c1d

Re-apply chart original scheme

12a9a39

github-actions bot added the plugins label May 15, 2024

Fix coverage

8bdbb2e

Merge branch 'master' of https://github.com/apache/superset into dieg…

98e620b

…o/ch79154/fix-color-inconsistency

Merge branch 'master' of https://github.com/apache/superset into dieg…

3e6c6a5

…o/ch79154/fix-color-inconsistency

michael-s-molina requested a review from justinpark May 31, 2024 17:43

Merge

636e9bd

geido merged commit 313ee59 into master Jun 20, 2024
34 checks passed

eschutho pushed a commit that referenced this pull request Jun 23, 2024

fix(Dashboard): Color inconsistency on refreshes and conflicts (#27439)

6ce959f

(cherry picked from commit 313ee59)

eschutho pushed a commit that referenced this pull request Jul 24, 2024

fix(Dashboard): Color inconsistency on refreshes and conflicts (#27439)

6a8535f

rusackas deleted the diego/ch79154/fix-color-inconsistency branch September 27, 2024 21:05

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.1.0 labels Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

geido commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

rusackas Mar 8, 2024

michael-s-molina Mar 8, 2024

geido Mar 8, 2024

villebro commented Mar 8, 2024

geido Mar 8, 2024

geido Mar 8, 2024 •

edited

Loading

geido commented Mar 8, 2024 •

edited

Loading

geido commented Mar 11, 2024 •

edited

Loading

geido commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

betodealmeida left a comment

kgabryje left a comment

mistercrunch commented May 10, 2024

geido commented May 15, 2024

github-actions bot commented May 15, 2024

geido commented May 28, 2024

github-actions bot commented May 28, 2024

sadpandajoe commented Jun 6, 2024

sadpandajoe commented Jun 6, 2024

geido commented Jun 20, 2024 •

edited

Loading

geido commented Jun 20, 2024

github-actions bot commented Jun 20, 2024

		@@ -84,6 +84,7 @@ These features flags currently default to True and **will be removed in a future

		[//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY"

		- AVOID_COLORS_COLLISION

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

fix(Dashboard): Color inconsistency on refreshes and conflicts #27439

Conversation

geido commented Mar 8, 2024 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

rusackas Mar 8, 2024

Choose a reason for hiding this comment

michael-s-molina Mar 8, 2024

Choose a reason for hiding this comment

geido Mar 8, 2024

Choose a reason for hiding this comment

villebro commented Mar 8, 2024

geido Mar 8, 2024

Choose a reason for hiding this comment

geido Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

geido commented Mar 8, 2024 • edited Loading

geido commented Mar 11, 2024 • edited Loading

geido commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

betodealmeida left a comment

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

mistercrunch commented May 10, 2024

geido commented May 15, 2024

github-actions bot commented May 15, 2024

geido commented May 28, 2024

github-actions bot commented May 28, 2024

sadpandajoe commented Jun 6, 2024

sadpandajoe commented Jun 6, 2024

geido commented Jun 20, 2024 • edited Loading

geido commented Jun 20, 2024

github-actions bot commented Jun 20, 2024

geido commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

geido Mar 8, 2024 •

edited

Loading

geido commented Mar 8, 2024 •

edited

Loading

geido commented Mar 11, 2024 •

edited

Loading

geido commented Jun 20, 2024 •

edited

Loading