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!

mistercrunch · 2024-03-25T16:43:24Z

Rebasing should fix the issues around required checks

geido · 2024-04-16T18:07:57Z

Thanks everyone for the reviews. After some more thinking, I will continue with the refactoring work on this branch so that we can merge a stable/longer term solution to this problem.

…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.

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

This was referenced Apr 16, 2024

[dashboard][color theme] chart color are changed when refreshing dashboard #16970

Closed

[dashboard][color theme] chart color are changed when refreshing dashboard #26456

Open

geido mentioned this pull request Apr 29, 2024

Charts Changing Color with Each Refresh #28104

Open

geido added 2 commits April 30, 2024 16:14

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

f7dc117

…o/ch79154/fix-color-inconsistency

Refactor: naming - centralize update [WIP]

fa0e830

geido mentioned this pull request May 2, 2024

Different colours for the same metrics #25384

Open

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

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

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

codecov bot commented Mar 8, 2024 •

edited

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

geido commented Mar 8, 2024 •

edited

geido commented Mar 11, 2024 •

edited

geido commented Mar 11, 2024

github-actions bot commented Mar 11, 2024

betodealmeida left a comment

kgabryje left a comment

mistercrunch commented Mar 25, 2024

geido commented Apr 16, 2024 •

edited

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

		@@ -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

Are you sure you want to change the base?

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

Conversation

geido commented Mar 8, 2024 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 8, 2024 • edited

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

Choose a reason for hiding this comment

geido commented Mar 8, 2024 • edited

geido commented Mar 11, 2024 • edited

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 Mar 25, 2024

geido commented Apr 16, 2024 • edited

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

geido commented Mar 8, 2024 •

edited

codecov bot commented Mar 8, 2024 •

edited

geido Mar 8, 2024 •

edited

geido commented Mar 8, 2024 •

edited

geido commented Mar 11, 2024 •

edited

geido commented Apr 16, 2024 •

edited