feat: improve color consistency

[stephenLYZ]

SUMMARY

This PR improves the existing color consistency. The main optimization is that in a dashboard if a color scheme is set for the dashboard (either in the color scheme select or via the metadata editor), all charts in the dashboard with the same label have the same color, while other labels are generated in the normal color scheme order.

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

[geido]

Hey @stephenLYZ thanks for working on this hard problem. Please read my comment. I am open to discuss this further.

[zhaoyongjie]

For testing purposes, used the virtual table as Dataset. We can see different value of column has different color:

SELECT * FROM 
(SELECT 'a' as color, 1 as val
UNION 
SELECT 'b', 1
UNION 
SELECT 'c', 1
UNION 
SELECT 'd', 1
UNION 
SELECT 'e', 1
UNION
SELECT 'f', 1
UNION 
SELECT 'g', 1
UNION 
SELECT 'h', 1
UNION 
SELECT 'i', 1
UNION
SELECT 'j', 1
UNION 
SELECT 'k', 1
UNION 
SELECT 'l', 1
UNION 
SELECT 'm', 1
UNION
SELECT 'n', 1
UNION 
SELECT 'o', 1
UNION 
SELECT 'p', 1
UNION 
SELECT 'q', 1
UNION
SELECT 'r', 1
UNION 
SELECT 's', 1
UNION 
SELECT 't', 1
UNION 
SELECT 'u', 1
UNION
SELECT 'v', 1
UNION
SELECT 'w', 1
UNION 
SELECT 'x', 1
UNION 
SELECT 'y', 1
UNION 
SELECT 'z', 1
) as _view
ORDER BY COLOR

1. Create virtual dataset in the SQLLab.
2. Create a Pie Chart 1) count(*) as metric 2) color as groupby 3) a,b,c in filter, save as new chart and create a dashboard.
3. Create a Pie Chart 1) count(*) as metric 2) color as groupby 3) d,e,f in filter, save as new chart and insert into previous dashboard.

In this PR

Current master(19daf65)

[geido]

For testing purposes, used the virtual table as Dataset. We can see different value of column has different color:

SELECT * FROM 
(SELECT 'a' as color, 1 as val
UNION 
SELECT 'b', 1
UNION 
SELECT 'c', 1
UNION 
SELECT 'd', 1
UNION 
SELECT 'e', 1
UNION
SELECT 'f', 1
UNION 
SELECT 'g', 1
UNION 
SELECT 'h', 1
UNION 
SELECT 'i', 1
UNION
SELECT 'j', 1
UNION 
SELECT 'k', 1
UNION 
SELECT 'l', 1
UNION 
SELECT 'm', 1
UNION
SELECT 'n', 1
UNION 
SELECT 'o', 1
UNION 
SELECT 'p', 1
UNION 
SELECT 'q', 1
UNION
SELECT 'r', 1
UNION 
SELECT 's', 1
UNION 
SELECT 't', 1
UNION 
SELECT 'u', 1
UNION
SELECT 'v', 1
UNION
SELECT 'w', 1
UNION 
SELECT 'x', 1
UNION 
SELECT 'y', 1
UNION 
SELECT 'z', 1
) as _view
ORDER BY COLOR

1. Create virtual dataset in the SQLLab.
2. Create a Pie Chart 1) count(*) as metric 2) color as groupby 3) a,b,c in filter, save as new chart and create a dashboard.
3. Create a Pie Chart 1) count(*) as metric 2) color as groupby 3) d,e,f in filter, save as new chart and insert into previous dashboard.

In this PR

Current master(19daf65)

You are right @zhaoyongjie that's because this PR brings back sharing the same color instance across all the charts of a dashboard, as I mentioned in a comment above. However, @stephenLYZ said that is for testing only, so I assume it will be reverted soon.

[stephenLYZ]

@geido I have actually reverted. 😂 I believe this PR should behave as expected. Since the column(label) of the second chart is different from the first one, they may not share the same color.

[geido]

@geido I have actually reverted. 😂 I believe this PR should behave as expected. Since the column(label) of the second chart is different from the first one, they may not share the same color.

@stephenLYZ but they have the same number of series, they should use the available colors of the color scheme from first to last, individually. What @zhaoyongjie is showing in master is the correct and expected behaviour afaik.

[geido]

Hey @stephenLYZ there is a lot of improvement that went into this. I am curious to see this in action. Do you think is it a good time to spin up a test env?

[stephenLYZ]

Hey @stephenLYZ there is a lot of improvement that went into this. I am curious to see this in action. Do you think is it a good time to spin up a test env?

I think it's okay and even though I didn't add tinyColor to resolve the color conflict, I don't think it gets in the way of the overall flow logic

[geido]

Hey @stephenLYZ there is a lot of improvement that went into this. I am curious to see this in action. Do you think is it a good time to spin up a test env?

I think it's okay and even though I didn't add tinyColor to resolve the color conflict, I don't think it gets in the way of the overall flow logic

Great. Let's make CI pass so that we can spin up the test env and start testing 🥳

[codecov]

Codecov Report

Merging #17792 (ba8385e) into master (bd63a1b) will increase coverage by 0.00%.
The diff coverage is 32.82%.

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

@@           Coverage Diff           @@
##           master   #17792   +/-   ##
=======================================
  Coverage   66.58%   66.58%           
=======================================
  Files        1641     1641           
  Lines       63533    63535    +2     
  Branches     6424     6424           
=======================================
+ Hits        42303    42305    +2     
  Misses      19551    19551           
  Partials     1679     1679           

Flag	Coverage Δ
hive	52.59% <50.00%> (-0.01%)	⬇️
mysql	81.83% <100.00%> (+<0.01%)	⬆️
postgres	81.88% <100.00%> (+<0.01%)	⬆️
presto	52.44% <50.00%> (-0.01%)	⬇️
python	82.31% <100.00%> (+<0.01%)	⬆️
sqlite	81.57% <100.00%> (+<0.01%)	⬆️

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

Impacted Files	Coverage Δ
...end/plugins/legacy-plugin-chart-chord/src/Chord.js	0.00% <0.00%> (ø)
...ns/legacy-plugin-chart-chord/src/transformProps.js	0.00% <0.00%> (ø)
.../legacy-plugin-chart-country-map/src/CountryMap.js	0.00% <0.00%> (ø)
...acy-plugin-chart-country-map/src/transformProps.js	0.00% <0.00%> (ø)
...ns/legacy-plugin-chart-histogram/src/Histogram.jsx	0.00% <0.00%> (ø)
...egacy-plugin-chart-histogram/src/transformProps.js	0.00% <0.00%> (ø)
...ins/legacy-plugin-chart-partition/src/Partition.js	0.00% <0.00%> (ø)
...egacy-plugin-chart-partition/src/transformProps.js	0.00% <0.00%> (ø)
...ntend/plugins/legacy-plugin-chart-rose/src/Rose.js	0.00% <0.00%> (ø)
...ins/legacy-plugin-chart-rose/src/transformProps.js	0.00% <0.00%> (ø)
... and 91 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd63a1b...ba02558. Read the comment docs.

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[geido]

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true FEATURE_UX_BETA=true

[github-actions]

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

[geido]

Hey @stephenLYZ I think I found a rather impactful issue.
Basically, when you change Dashboard, the color of the labels is persisted and applied also to other Dashboards. You can see that those get correctly removed only when hard-refreshing the page.

Superset.mp4

[geido]

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true FEATURE_UX_BETA=true

[github-actions]

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

[geido]

Hey @stephenLYZ I found that two labels within the same chart share exactly the same color. This reminded me of the color conflict resolution requirements that I think we skipped to prioritize testing looking at the threads above. What do you think?

[stephenLYZ]

Discussion result with @geido :
using tinycolor.analogous to generate similar colors for each color scheme, for example, if our color scheme is [A, B, C] then the generated colors are [A1, B1, C1, A2, B2, C2 ....]. Here the number of color labels is always less than or equal to the generated list of colors, in order to prevent getting the same colors.

[geido]

Discussion result with @geido :
using tinycolor.analogous to generate similar colors for each color scheme, for example, if our color scheme is [A, B, C] then the generated colors are [A1, B1, C1, A2, B2, C2 ....]. Here the number of color labels is always less than or equal to the generated list of colors, in order to prevent getting the same colors.

Just for the sake of clarity, I am adding a bit more info here.

Let's say that we have Chart A and Chart B that share two labels. The same color for both labels would be set, but now Chart B has an adjacent label that also uses the same color. That's the main color conflict we need to resolve here.

The solution that Stephen is proposing above, generates new colors for the shared_label_colors using the analogous utility. That would assure (in most cases) that two adjacent labels would not use exactly the same color.

I think the solution works on paper, but I'd like to hear feedback from @rusackas @zhaoyongjie @jinghua-qa

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.

[geido]

/testenv up FEATURE_DASHBOARD_NATIVE_FILTERS=true FEATURE_DASHBOARD_CROSS_FILTERS=true FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true FEATURE_UX_BETA=true

[github-actions]

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

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.