fix: repeated color in the same chart #23762

lilykuang · 2023-04-21T01:00:11Z

SUMMARY

Currently, the charts are experiencing an issue where colors are being repeated even though the color palette has more colors than the chart requires. This is problematic because it can make it difficult for users to distinguish between different data points and can lead to confusion. the code will now check colors in shared label and remove the color for the list.

add a feature flag
implement a function to remove shared label color from range

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

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

eschutho · 2023-05-01T19:47:33Z

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

+        sharedColorMap.forEach((value, key) => {
+          if (key !== cleanedValue) {
+            const index = updatedRange.indexOf(value);
+            updatedRange.splice(index, 1);


It looks like this may be removing items from a list. Can you use the filter function here instead?

eschutho · 2023-05-01T20:17:08Z

/testenv up

github-actions · 2023-05-01T20:17:59Z

@eschutho Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2023-05-01T20:18:00Z

@eschutho Ephemeral environment creation failed. Please check the Actions logs for details.

codecov · 2023-05-13T06:59:47Z

Codecov Report

Merging #23762 (daaddac) into master (1670275) will increase coverage by 0.00%.
The diff coverage is 65.38%.

❗ Current head daaddac differs from pull request most recent head d5d68b7. Consider uploading reports for the commit d5d68b7 to get more accurate results

@@           Coverage Diff           @@
##           master   #23762   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files        1942     1942           
  Lines       75205    75226   +21     
  Branches     8145     8148    +3     
=======================================
+ Hits        51314    51329   +15     
- Misses      21806    21812    +6     
  Partials     2085     2085

Flag	Coverage Δ
hive	`53.16% <15.38%> (-0.02%)`	⬇️
javascript	`54.53% <84.61%> (+0.01%)`	⬆️
mysql	`78.91% <15.38%> (-0.03%)`	⬇️
postgres	`78.99% <15.38%> (-0.03%)`	⬇️
presto	`53.09% <46.15%> (-0.01%)`	⬇️
python	`82.77% <46.15%> (-0.02%)`	⬇️
sqlite	`77.51% <15.38%> (-0.03%)`	⬇️
unit	`53.03% <15.38%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...n-chart-handlebars/src/plugin/controls/columns.tsx	`15.78% <0.00%> (+0.78%)`	⬆️
superset/config.py	`92.02% <ø> (ø)`
superset/models/helpers.py	`69.11% <45.45%> (-0.31%)`	⬇️
superset/models/core.py	`89.97% <50.00%> (-0.19%)`	⬇️
...uperset-ui-core/src/color/CategoricalColorScale.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

eschutho

LGTM!

geido · 2024-01-05T15:32:13Z

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

@@ -96,7 +114,12 @@ class CategoricalColorScale extends ExtensibleFunction {
    const newColor = this.scale(cleanedValue);


This is moving the scale to the next color in order

geido · 2024-01-05T15:32:34Z

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

@@ -96,7 +114,12 @@ class CategoricalColorScale extends ExtensibleFunction {
    const newColor = this.scale(cleanedValue);
    if (!color) {
      color = newColor;
+      if (isFeatureEnabled(FeatureFlag.AVOID_COLORS_COLLISION)) {
+        this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue);
+        color = this.scale(cleanedValue);


Calling this again will move to the next color in the scale again unnecessarily

lilykuang requested a review from stephenLYZ April 21, 2023 01:00

pull-request-size bot added the size/S label Apr 21, 2023

lilykuang requested review from kgabryje and geido April 21, 2023 01:00

lilykuang force-pushed the lily/repeated-color branch from 52debb3 to e72bbb3 Compare April 21, 2023 04:04

lilykuang requested review from jinghua-qa, eschutho, rusackas and betodealmeida as code owners April 26, 2023 23:59

pull-request-size bot added size/M and removed size/S labels Apr 26, 2023

Lily Kuang added 6 commits April 26, 2023 17:09

fix: avoid color collision in chart

443f913

added comments

fea753a

repeated color when there is none

2d0f564

fix tests

8c7fc74

update color consistency e2e tests

bd4679d

update e2e

844bee8

lilykuang force-pushed the lily/repeated-color branch from 5dfbdb6 to 844bee8 Compare April 27, 2023 00:09

fix e2e

2c6e38f

eschutho reviewed May 1, 2023

View reviewed changes

Lily Kuang added 2 commits May 12, 2023 14:47

Merge remote-tracking branch 'origin/master' into lily/repeated-color

14636cd

add test

f978b1f

pull-request-size bot added size/L and removed size/M labels May 13, 2023

add feature flag AVOID_COLORS_COLLISION and update e2e tests

d5d68b7

lilykuang force-pushed the lily/repeated-color branch from 6899cc4 to d5d68b7 Compare May 16, 2023 01:03

lilykuang requested a review from eschutho May 17, 2023 17:24

eschutho approved these changes May 19, 2023

View reviewed changes

lilykuang merged commit 66594ad into apache:master May 19, 2023

lilykuang deleted the lily/repeated-color branch May 19, 2023 21:48

lilykuang mentioned this pull request Jul 11, 2023

fix: color collision in dashboard with tabs #24670

Merged

9 tasks

geido reviewed Jan 5, 2024

View reviewed changes

geido mentioned this pull request Mar 8, 2024

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

Merged

9 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: repeated color in the same chart #23762

fix: repeated color in the same chart #23762

lilykuang commented Apr 21, 2023 •

edited

Loading

eschutho May 1, 2023 •

edited

Loading

eschutho commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

codecov bot commented May 13, 2023 •

edited

Loading

eschutho left a comment

geido Jan 5, 2024

geido Jan 5, 2024

		@@ -96,7 +114,12 @@ class CategoricalColorScale extends ExtensibleFunction {
		const newColor = this.scale(cleanedValue);

fix: repeated color in the same chart #23762

fix: repeated color in the same chart #23762

Conversation

lilykuang commented Apr 21, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

eschutho May 1, 2023 • edited Loading

Choose a reason for hiding this comment

eschutho commented May 1, 2023

github-actions bot commented May 1, 2023

github-actions bot commented May 1, 2023

codecov bot commented May 13, 2023 • edited Loading

Codecov Report

eschutho left a comment

Choose a reason for hiding this comment

geido Jan 5, 2024

Choose a reason for hiding this comment

geido Jan 5, 2024

Choose a reason for hiding this comment

lilykuang commented Apr 21, 2023 •

edited

Loading

eschutho May 1, 2023 •

edited

Loading

codecov bot commented May 13, 2023 •

edited

Loading