You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by di...@apache.org on 2024/03/08 15:08:53 UTC
(superset) 04/04: Update map on hidden tabs
This is an automated email from the ASF dual-hosted git repository.
diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6f89a9ca0e0808cdf0b9a564db65f09c9e20d11e
Author: geido <di...@gmail.com>
AuthorDate: Fri Mar 8 16:08:34 2024 +0100
Update map on hidden tabs
---
RESOURCES/FEATURE_FLAGS.md | 1 +
.../cypress/e2e/dashboard/editmode.test.ts | 56 ++++---
.../src/color/CategoricalColorScale.ts | 149 +++++++++++-------
.../src/color/SharedLabelColorSingleton.ts | 19 +--
.../test/color/CategoricalColorNameSpace.test.ts | 2 +-
.../test/color/CategoricalColorScale.test.ts | 167 ++++++++++-----------
.../test/color/SharedLabelColorSingleton.test.ts | 16 +-
.../src/dashboard/actions/dashboardInfo.ts | 13 +-
.../src/dashboard/components/Header/index.jsx | 10 +-
.../dashboard/components/PropertiesModal/index.tsx | 7 +-
.../dashboard/components/gridComponents/Tabs.jsx | 51 ++++++-
.../dashboard/containers/DashboardComponent.jsx | 1 +
.../src/dashboard/containers/DashboardPage.tsx | 1 +
.../components/ExploreChartHeader/index.jsx | 23 ++-
tests/integration_tests/superset_test_config.py | 1 +
15 files changed, 296 insertions(+), 221 deletions(-)
diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md
index 119988653d..1bbf24f4be 100644
--- a/RESOURCES/FEATURE_FLAGS.md
+++ b/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
- DASHBOARD_CROSS_FILTERS
- ENABLE_JAVASCRIPT_CONTROLS
- KV_STORE
diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
index 28d5c166f5..e50bf52be9 100644
--- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
+++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts
@@ -19,7 +19,12 @@
import { SAMPLE_DASHBOARD_1, TABBED_DASHBOARD } from 'cypress/utils/urls';
import { drag, resize, waitForChartLoad } from 'cypress/utils';
import * as ace from 'brace';
-import { interceptGet, interceptUpdate, openTab } from './utils';
+import {
+ interceptGet,
+ interceptUpdate,
+ openTab,
+ interceptGet as interceptDashboardGet,
+} from './utils';
import {
interceptExploreJson,
interceptFiltering as interceptCharts,
@@ -124,7 +129,7 @@ function selectColorScheme(color: string) {
)
.first()
.click();
- cy.getBySel(color).click();
+ cy.getBySel(color).click({ force: true });
}
function applyChanges() {
@@ -169,6 +174,7 @@ function writeMetadata(metadata: string) {
function openExplore(chartName: string) {
interceptExploreJson();
+ interceptDashboardGet();
cy.get(
`[data-test-chart-name='${chartName}'] [aria-label='More Options']`,
@@ -210,7 +216,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
});
it('should apply same color to same labels with color scheme set', () => {
@@ -231,7 +237,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open 2nd main tab
openTab(0, 1);
@@ -240,7 +246,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
});
it('should apply same color to same labels with no color scheme set', () => {
@@ -261,7 +267,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
// open 2nd main tab
openTab(0, 1);
@@ -270,7 +276,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
});
it('custom label colors should take the precedence in nested tabs', () => {
@@ -384,17 +390,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
- .should('have.css', 'fill', 'rgb(224, 67, 85)');
+ .should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
- .should('have.css', 'fill', 'rgb(163, 143, 121)');
+ .should('have.css', 'fill', 'rgb(90, 193, 137)');
openProperties();
cy.get('[aria-label="Select color scheme"]').should('have.value', '');
@@ -423,17 +429,17 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(69, 78, 124)');
+ .should('have.css', 'fill', 'rgb(31, 168, 201)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(1)
- .should('have.css', 'fill', 'rgb(224, 67, 85)');
+ .should('have.css', 'fill', 'rgb(69, 78, 124)');
cy.get(
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.eq(2)
- .should('have.css', 'fill', 'rgb(163, 143, 121)');
+ .should('have.css', 'fill', 'rgb(90, 193, 137)');
});
it('should show the same colors in Explore', () => {
@@ -459,12 +465,6 @@ describe('Dashboard edit', () => {
)
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
- // label Christopher
- cy.get(
- '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
- )
- .eq(1)
- .should('have.css', 'fill', 'rgb(172, 32, 119)');
openExplore('Top 10 California Names Timeseries');
@@ -472,10 +472,6 @@ describe('Dashboard edit', () => {
cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
.first()
.should('have.css', 'fill', 'rgb(255, 0, 0)');
- // label Christopher
- cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
- .eq(1)
- .should('have.css', 'fill', 'rgb(172, 32, 119)');
});
it('should change color scheme multiple times', () => {
@@ -496,7 +492,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open 2nd main tab
openTab(0, 1);
@@ -505,7 +501,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
editDashboard();
openProperties();
@@ -516,7 +512,7 @@ describe('Dashboard edit', () => {
// label Anthony
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.eq(2)
- .should('have.css', 'fill', 'rgb(244, 176, 42)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
// open main tab and nested tab
openTab(0, 0);
@@ -527,7 +523,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(244, 176, 42)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
});
it('should apply the color scheme across main tabs', () => {
@@ -542,7 +538,7 @@ describe('Dashboard edit', () => {
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
});
it('should apply the color scheme across main tabs for rendered charts', () => {
@@ -558,7 +554,7 @@ describe('Dashboard edit', () => {
cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
.first()
- .should('have.css', 'fill', 'rgb(156, 52, 152)');
+ .should('have.css', 'fill', 'rgb(41, 105, 107)');
// change scheme now that charts are rendered across the main tabs
editDashboard();
@@ -588,7 +584,7 @@ describe('Dashboard edit', () => {
'[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
)
.first()
- .should('have.css', 'fill', 'rgb(51, 61, 71)');
+ .should('have.css', 'fill', 'rgb(234, 11, 140)');
// open another nested tab
openTab(2, 1);
diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
index c4e96a9a1f..4223fae776 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts
@@ -17,11 +17,13 @@ class CategoricalColorScale extends ExtensibleFunction {
colors: string[];
+ expandedColors: string[];
+
scale: ScaleOrdinal<{ toString(): string }, string>;
forcedColors: ColorsLookup;
- colorUsageCount: Map<string, number>;
+ sharedColorMapInstance: ReturnType<typeof getSharedLabelColor>;
sliceMap: Map<string, string>;
@@ -35,8 +37,17 @@ class CategoricalColorScale extends ExtensibleFunction {
*/
constructor(colors: string[], forcedColors: ColorsInitLookup = {}) {
super((value: string, sliceId?: number) => this.getColor(value, sliceId));
+ // holds original color scheme colors
this.originColors = colors;
- this.colors = colors;
+ // holds the extended color range (includes analagous colors)
+ this.expandedColors = colors;
+ // holds the values of this specific slice (label+color)
+ this.sliceMap = new Map();
+ // shared color map instance (when context is shared, i.e. dashboard)
+ this.sharedColorMapInstance = getSharedLabelColor();
+ // holds the multiple value for analogous colors range
+ this.multiple = 0;
+
this.scale = scaleOrdinal<{ toString(): string }, string>();
this.scale.range(colors);
@@ -49,18 +60,12 @@ class CategoricalColorScale extends ExtensibleFunction {
});
// forced colors from parent (usually CategoricalColorNamespace)
+ // currently used in dashboards to set custom label colors
this.forcedColors = forcedColors as ColorsLookup;
- // holds the usage count for each color including forced colors from parent
- this.colorUsageCount = this.initColorUsageCounter();
- // holds the values in this specific slice
- this.sliceMap = new Map();
- // holds the multiple value for analogous colors range
- this.multiple = 0;
}
/**
* Increments the color range with analogous colors
- *
*/
incrementColorRange() {
const multiple = Math.floor(
@@ -71,34 +76,24 @@ class CategoricalColorScale extends ExtensibleFunction {
if (multiple > this.multiple) {
this.multiple = multiple;
const newRange = getAnalogousColors(this.originColors, multiple);
- this.range(this.originColors.concat(newRange));
- // update the colorUsageCount map with the new colors
- newRange.forEach(color => {
- this.colorUsageCount.set(color, 0);
- });
+ const expandedColors = this.originColors.concat(newRange);
+
+ this.range(expandedColors);
+ this.expandedColors = expandedColors;
}
}
/**
- * Initializes the color usage count map
- * @returns a map of color to usage count
+ * Get the color for a given value
+ * @param value
+ * @param sliceId
+ * @returns the color for the given value
*/
- initColorUsageCounter(): Map<string, number> {
- const colorUsageCount = new Map(this.colors.map(color => [color, 0]));
- // add forced colors to the usage count map
- // helps reducing conflicts but increases randomness
- Object.values(this.forcedColors).forEach(color => {
- const count = colorUsageCount.get(color) || 0;
- colorUsageCount.set(color, count + 1);
- });
- return colorUsageCount;
- }
-
getColor(value?: string, sliceId?: number): string {
const cleanedValue = stringifyAndTrim(value);
- const sharedLabelColor = getSharedLabelColor();
- const sharedColorMap = sharedLabelColor.getColorMap();
+ const sharedColorMap = this.sharedColorMapInstance.getColorMap();
const sharedColor = sharedColorMap.get(cleanedValue);
+ // priority: forced color (example: custom label colors) > shared color > scale color
const forcedColor = this.forcedColors?.[cleanedValue] || sharedColor;
let color = forcedColor || this.scale(cleanedValue);
@@ -107,22 +102,24 @@ class CategoricalColorScale extends ExtensibleFunction {
if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) {
this.incrementColorRange();
}
- if (this.isColorUsed(color)) {
- // color was used in this slice already
+ if (
+ // feature flag to be deprecated (will become standard behaviour)
+ isFeatureEnabled(FeatureFlag.AvoidColorsCollision) &&
+ this.isColorUsed(color)
+ ) {
// fallback to least used color
color = this.getNextAvailableColor(color);
}
}
- // increment the usage count for the color
- this.incrementColorUsage(color);
-
- // store the value+color in the shared map for dashboard consistency
- sharedLabelColor.addSlice(cleanedValue, color, sliceId);
-
// keep track of values in this slice
this.sliceMap.set(cleanedValue, color);
+ // store the value+color in the shared context
+ if (sliceId) {
+ this.sharedColorMapInstance.addSlice(cleanedValue, color, sliceId);
+ }
+
return color;
}
@@ -132,30 +129,69 @@ class CategoricalColorScale extends ExtensibleFunction {
* @returns whether the color is used in this slice
*/
isColorUsed(color: string): boolean {
- return Array.from(this.sliceMap.values()).includes(color);
+ return this.getColorUsageCount(color) > 0;
+ }
+
+ /**
+ * Get the count of the color usage in this slice
+ * @param sliceId
+ * @param color
+ * @returns the count of the color usage in this slice
+ */
+ getColorUsageCount(currentColor: string): number {
+ let count = 0;
+ this.sliceMap.forEach(color => {
+ if (color === currentColor) {
+ count += 1;
+ }
+ });
+ return count;
}
/**
* Lower chances of color collision by returning the least used color
- * Checks across colors of current slice and forced colors (all slices from parent)
+ * Checks across colors of current slice within SharedLabelColorSingleton
*
- * @param excludeColor
+ * @param currentColor
* @returns the least used color that is not the excluded color
*/
- getNextAvailableColor(excludeColor: string) {
- // sort the colors by count, then exclude the specified color
- const sortedColors = [...this.colorUsageCount.entries()]
- .sort((a, b) => a[1] - b[1])
- // eslint-disable-next-line @typescript-eslint/no-unused-vars
- .filter(([color, _]) => color !== excludeColor);
- const color = sortedColors[0][0];
+ getNextAvailableColor(currentColor: string) {
+ const colorUsageArray = this.expandedColors.map(color => ({
+ color,
+ count: this.getColorUsageCount(color),
+ }));
+ const currentColorCount = this.getColorUsageCount(currentColor);
+ const otherColors = colorUsageArray.filter(
+ colorEntry => colorEntry.color !== currentColor,
+ );
+ // all other colors are used as much or more than currentColor
+ const hasNoneAvailable = otherColors.every(
+ colorEntry => colorEntry.count >= currentColorCount,
+ );
- return color;
+ // fallback to currentColor color
+ if (!otherColors.length || hasNoneAvailable) {
+ return currentColor;
+ }
+
+ // Finding the least used color
+ const leastUsedColor = otherColors.reduce((min, entry) =>
+ entry.count < min.count ? entry : min,
+ ).color;
+
+ return leastUsedColor;
}
- incrementColorUsage(color: string) {
- const currentCount = this.colorUsageCount.get(color) || 0;
- this.colorUsageCount.set(color, currentCount + 1);
+ /**
+ * Enforce specific color for a given value at the scale level
+ * Overrides any existing color and forced color for the given value
+ *
+ * @param {*} value value
+ * @param {*} forcedColor forcedColor
+ */
+ setColor(value: string, forcedColor: string) {
+ this.forcedColors[stringifyAndTrim(value)] = forcedColor;
+ return this;
}
/**
@@ -163,11 +199,15 @@ class CategoricalColorScale extends ExtensibleFunction {
* @returns an object where the key is the data value and the value is the hex color code
*/
getColorMap() {
- const colorMap = {};
+ const colorMap: { [key: string]: string | undefined } = {};
this.scale.domain().forEach(value => {
colorMap[value.toString()] = this.scale(value);
});
- return { ...colorMap, ...this.forcedColors };
+
+ return {
+ ...colorMap,
+ ...this.forcedColors,
+ };
}
/**
@@ -179,7 +219,6 @@ class CategoricalColorScale extends ExtensibleFunction {
this.forcedColors,
);
copy.forcedColors = { ...this.forcedColors };
- copy.colorUsageCount = new Map(this.colorUsageCount);
copy.domain(this.domain());
copy.unknown(this.unknown());
return copy;
@@ -225,7 +264,7 @@ class CategoricalColorScale extends ExtensibleFunction {
return this.scale.range();
}
- this.colors = newRange;
+ this.expandedColors = newRange;
this.scale.range(newRange);
return this;
}
diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
index 4837d0f362..05a56de129 100644
--- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
+++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts
@@ -43,10 +43,10 @@ export class SharedLabelColor {
CategoricalColorNamespace.getNamespace(colorNamespace);
const newColorMap = new Map();
this.colorMap.clear();
- this.sliceLabelMap.forEach(labels => {
+ this.sliceLabelMap.forEach((labels, sliceId) => {
const colorScale = categoricalNamespace.getScale(colorScheme);
labels.forEach(label => {
- const newColor = colorScale(label);
+ const newColor = colorScale.getColor(label, sliceId);
newColorMap.set(label, newColor);
});
});
@@ -57,12 +57,8 @@ export class SharedLabelColor {
return this.colorMap;
}
- addSlice(label: string, color: string, sliceId?: number) {
- if (
- this.source !== SharedLabelColorSource.Dashboard ||
- sliceId === undefined
- )
- return;
+ addSlice(label: string, color: string, sliceId: number) {
+ if (this.source !== SharedLabelColorSource.Dashboard) return;
const labels = this.sliceLabelMap.get(sliceId) || [];
if (!labels.includes(label)) {
labels.push(label);
@@ -83,13 +79,6 @@ export class SharedLabelColor {
this.colorMap = newColorMap;
}
- reset() {
- const copyColorMap = new Map(this.colorMap);
- copyColorMap.forEach((_, label) => {
- this.colorMap.set(label, '');
- });
- }
-
clear() {
this.sliceLabelMap.clear();
this.colorMap.clear();
diff --git a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
index d5e1ef8f0b..69fb38eea3 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts
@@ -98,7 +98,7 @@ describe('CategoricalColorNamespace', () => {
namespace.setColor('dog', 'black');
const scale = namespace.getScale('testColors');
scale.setColor('dog', 'pink');
- expect(scale.getColor('dog')).toBe('black');
+ expect(scale.getColor('dog')).toBe('pink');
expect(scale.getColor('boy')).not.toBe('black');
});
it('does not affect scales in other namespaces', () => {
diff --git a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
index 48e43d8351..987c83fbc5 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts
@@ -18,50 +18,49 @@
*/
import { ScaleOrdinal } from 'd3-scale';
-import {
- CategoricalColorScale,
- FeatureFlag,
- getSharedLabelColor,
-} from '@superset-ui/core';
+import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core';
describe('CategoricalColorScale', () => {
it('exists', () => {
expect(CategoricalColorScale !== undefined).toBe(true);
});
- describe('new CategoricalColorScale(colors, parentForcedColors)', () => {
- it('can create new scale when parentForcedColors is not given', () => {
+ describe('new CategoricalColorScale(colors, forcedColors)', () => {
+ it('can create new scale when forcedColors is not given', () => {
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
expect(scale).toBeInstanceOf(CategoricalColorScale);
});
- it('can create new scale when parentForcedColors is given', () => {
- const parentForcedColors = {};
+ it('can create new scale when forcedColors is given', () => {
+ const forcedColors = {};
const scale = new CategoricalColorScale(
['blue', 'red', 'green'],
- parentForcedColors,
+ forcedColors,
);
expect(scale).toBeInstanceOf(CategoricalColorScale);
- expect(scale.parentForcedColors).toBe(parentForcedColors);
+ expect(scale.forcedColors).toBe(forcedColors);
});
it('can refer to colors based on their index', () => {
- const parentForcedColors = { pig: 1, horse: 5 };
+ const forcedColors = { pig: 1, horse: 5 };
const scale = new CategoricalColorScale(
['blue', 'red', 'green'],
- parentForcedColors,
+ forcedColors,
);
expect(scale.getColor('pig')).toEqual('red');
- expect(parentForcedColors.pig).toEqual('red');
+ expect(forcedColors.pig).toEqual('red');
// can loop around the scale
expect(scale.getColor('horse')).toEqual('green');
- expect(parentForcedColors.horse).toEqual('green');
+ expect(forcedColors.horse).toEqual('green');
});
});
describe('.getColor(value)', () => {
it('returns same color for same value', () => {
- const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ const scale = new CategoricalColorScale(['blue', 'red', 'green'], {
+ pig: 'red',
+ horse: 'green',
+ });
const c1 = scale.getColor('pig');
const c2 = scale.getColor('horse');
const c3 = scale.getColor('pig');
@@ -118,26 +117,8 @@ describe('CategoricalColorScale', () => {
scale.getColor('cow');
scale.getColor('donkey');
scale.getColor('goat');
- expect(scale.range()).toHaveLength(6);
+ expect(scale.range()).toHaveLength(9);
});
-
- it('should remove shared color from range if avoid colors collision enabled', () => {
- window.featureFlags = {
- [FeatureFlag.AvoidColorsCollision]: true,
- };
- const scale = new CategoricalColorScale(['blue', 'red', 'green']);
- const color1 = scale.getColor('a', 1);
- expect(scale.range()).toHaveLength(3);
- const color2 = scale.getColor('a', 2);
- expect(color1).toBe(color2);
- scale.getColor('b', 2);
- expect(scale.range()).toHaveLength(2);
- scale.getColor('c', 2);
- expect(scale.range()).toHaveLength(1);
- });
- window.featureFlags = {
- [FeatureFlag.AvoidColorsCollision]: false,
- };
});
describe('.setColor(value, forcedColor)', () => {
it('overrides default color', () => {
@@ -145,16 +126,14 @@ describe('CategoricalColorScale', () => {
scale.setColor('pig', 'pink');
expect(scale.getColor('pig')).toBe('pink');
});
- it('does not override parentForcedColors', () => {
+ it('does override forcedColors', () => {
const scale1 = new CategoricalColorScale(['blue', 'red', 'green']);
scale1.setColor('pig', 'black');
- const scale2 = new CategoricalColorScale(
- ['blue', 'red', 'green'],
- scale1.forcedColors,
- );
+
+ const scale2 = new CategoricalColorScale(['blue', 'red', 'green']);
scale2.setColor('pig', 'pink');
+ expect(scale2.getColor('pig')).toBe('pink');
expect(scale1.getColor('pig')).toBe('black');
- expect(scale2.getColor('pig')).toBe('black');
});
it('returns the scale', () => {
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
@@ -163,7 +142,7 @@ describe('CategoricalColorScale', () => {
});
});
describe('.getColorMap()', () => {
- it('returns correct mapping and parentForcedColors and forcedColors are specified', () => {
+ it('returns correct mapping using least used color', () => {
const scale1 = new CategoricalColorScale(['blue', 'red', 'green']);
scale1.setColor('cow', 'black');
const scale2 = new CategoricalColorScale(
@@ -177,7 +156,7 @@ describe('CategoricalColorScale', () => {
expect(scale2.getColorMap()).toEqual({
cow: 'black',
pig: 'pink',
- horse: 'green',
+ horse: 'blue', // least used color
});
});
});
@@ -230,64 +209,72 @@ describe('CategoricalColorScale', () => {
});
describe('a CategoricalColorScale instance is also a color function itself', () => {
- it('scale(value) returns color similar to calling scale.getColor(value)', () => {
+ it('scale(value) returns least used color', () => {
+ window.featureFlags = {
+ [FeatureFlag.AvoidColorsCollision]: true,
+ };
const scale = new CategoricalColorScale(['blue', 'red', 'green']);
- expect(scale.getColor('pig')).toBe(scale('pig'));
- expect(scale.getColor('cat')).toBe(scale('cat'));
+ expect(scale.getColor('pig')).toBe('blue');
+ expect(scale('pig')).toBe('red'); // blue is now used and red is next available
+ expect(scale.getColor('cat')).toBe('blue'); // pig got red, now blue is available
+ expect(scale('cat')).toBe('green'); // blue is now used and green is next available
});
- });
-
- describe("is compatible with D3's ScaleOrdinal", () => {
- it('passes type check', () => {
- const scale: ScaleOrdinal<{ toString(): string }, string> =
- new CategoricalColorScale(['blue', 'red', 'green']);
+ it('scale(value) returns same color for same value', () => {
+ window.featureFlags = {
+ [FeatureFlag.AvoidColorsCollision]: false,
+ };
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ expect(scale.getColor('pig')).toBe('blue');
expect(scale('pig')).toBe('blue');
+ expect(scale.getColor('cat')).toBe('red');
+ expect(scale('cat')).toBe('red');
});
});
- describe('.removeSharedLabelColorFromRange(colorMap, cleanedValue)', () => {
- it('should remove shared color from range', () => {
- const scale = new CategoricalColorScale(['blue', 'green', 'red']);
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
+ describe('.getNextAvailableColor(currentColor)', () => {
+ it('returns the current color if it is the least used or equally used among colors', () => {
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ scale.getColor('cat');
+ scale.getColor('dog');
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.clear();
- const colorMap = sharedLabelColor.getColorMap();
- sharedLabelColor.addSlice('cow', 'blue', 1);
- scale.removeSharedLabelColorFromRange(colorMap, 'pig');
- expect(scale.range()).toEqual(['green', 'red']);
- scale.removeSharedLabelColorFromRange(colorMap, 'cow');
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- sharedLabelColor.clear();
+ // Since 'green' hasn't been used, it's considered the least used.
+ expect(scale.getNextAvailableColor('blue')).toBe('green');
});
- it('recycles colors when all colors are in sharedLabelColor', () => {
- const scale = new CategoricalColorScale(['blue', 'green', 'red']);
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- const sharedLabelColor = getSharedLabelColor();
- const colorMap = sharedLabelColor.getColorMap();
- sharedLabelColor.addSlice('cow', 'blue', 1);
- sharedLabelColor.addSlice('pig', 'red', 1);
- sharedLabelColor.addSlice('horse', 'green', 1);
- scale.removeSharedLabelColorFromRange(colorMap, 'goat');
- expect(scale.range()).toEqual(['blue', 'green', 'red']);
- sharedLabelColor.clear();
+ it('handles cases where all colors are equally used and returns the current color', () => {
+ const scale = new CategoricalColorScale(['blue', 'red', 'green']);
+ scale.getColor('cat'); // blue
+ scale.getColor('dog'); // red
+ scale.getColor('fish'); // green
+ // All colors used once, so the function should return the current color
+ expect(scale.getNextAvailableColor('red')).toBe('red');
});
- it('should remove parentForcedColors from range', () => {
- const parentForcedColors = { house: 'blue', cow: 'red' };
- const scale = new CategoricalColorScale(
- ['blue', 'red', 'green'],
- parentForcedColors,
- );
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.clear();
- const colorMap = sharedLabelColor.getColorMap();
- scale.removeSharedLabelColorFromRange(colorMap, 'pig');
- expect(scale.range()).toEqual(['green']);
- scale.removeSharedLabelColorFromRange(colorMap, 'cow');
- expect(scale.range()).toEqual(['red', 'green']);
- sharedLabelColor.clear();
+ it('returns the least used color accurately even when some colors are used more frequently', () => {
+ const scale = new CategoricalColorScale([
+ 'blue',
+ 'red',
+ 'green',
+ 'yellow',
+ ]);
+ scale.getColor('cat'); // blue
+ scale.getColor('dog'); // red
+ scale.getColor('frog'); // green
+ scale.getColor('fish'); // yellow
+ scale.getColor('goat'); // blue
+ scale.getColor('horse'); // red
+ scale.getColor('pony'); // green
+
+ // Yellow is the least used color, so it should be returned.
+ expect(scale.getNextAvailableColor('blue')).toBe('yellow');
+ });
+ });
+
+ describe("is compatible with D3's ScaleOrdinal", () => {
+ it('passes type check', () => {
+ const scale: ScaleOrdinal<{ toString(): string }, string> =
+ new CategoricalColorScale(['blue', 'red', 'green']);
+ expect(scale('pig')).toBe('blue');
});
});
});
diff --git a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
index 8f89ae8a69..ae23f34cc4 100644
--- a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts
@@ -96,13 +96,7 @@ describe('SharedLabelColor', () => {
it('should do nothing when source is not dashboard', () => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.source = SharedLabelColorSource.Explore;
- sharedLabelColor.addSlice('a', 'red');
- expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({});
- });
-
- it('should do nothing when sliceId is undefined', () => {
- const sharedLabelColor = getSharedLabelColor();
- sharedLabelColor.addSlice('a', 'red');
+ sharedLabelColor.addSlice('a', 'red', 1);
expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({});
});
});
@@ -144,8 +138,8 @@ describe('SharedLabelColor', () => {
const colorMap = sharedLabelColor.getColorMap();
expect(Object.fromEntries(colorMap)).toEqual({
a: 'yellow',
- b: 'yellow',
- c: 'green',
+ b: 'green',
+ c: 'yellow',
});
});
@@ -195,9 +189,9 @@ describe('SharedLabelColor', () => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.addSlice('a', 'red', 1);
sharedLabelColor.addSlice('b', 'blue', 2);
- sharedLabelColor.reset();
+ sharedLabelColor.clear();
const colorMap = sharedLabelColor.getColorMap();
- expect(Object.fromEntries(colorMap)).toEqual({ a: '', b: '' });
+ expect(Object.fromEntries(colorMap)).toEqual({});
});
});
});
diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts
index 472f945b54..36c2f02f61 100644
--- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts
+++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts
@@ -54,16 +54,15 @@ export function dashboardInfoChanged(newInfo: { metadata: any }) {
const categoricalNamespace = CategoricalColorNamespace.getNamespace(
metadata?.color_namespace,
);
-
+ // reset forced colors
categoricalNamespace.resetColors();
- if (metadata?.shared_label_colors) {
- updateColorSchema(metadata, metadata?.shared_label_colors);
- }
+ const mergedLabelColors = {
+ ...(metadata?.label_colors || {}),
+ ...(metadata?.shared_label_colors || {}),
+ };
- if (metadata?.label_colors) {
- updateColorSchema(metadata, metadata?.label_colors);
- }
+ updateColorSchema(metadata, mergedLabelColors);
return { type: DASHBOARD_INFO_UPDATED, newInfo };
}
diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx
index 485185d607..e924d22770 100644
--- a/superset-frontend/src/dashboard/components/Header/index.jsx
+++ b/superset-frontend/src/dashboard/components/Header/index.jsx
@@ -372,13 +372,17 @@ class Header extends React.PureComponent {
? currentRefreshFrequency
: dashboardInfo.metadata?.refresh_frequency;
- const currentColorScheme =
- dashboardInfo?.metadata?.color_scheme || colorScheme;
const currentColorNamespace =
dashboardInfo?.metadata?.color_namespace || colorNamespace;
+ const currentColorScheme =
+ dashboardInfo?.metadata?.color_scheme || colorScheme;
const currentSharedLabelColors = Object.fromEntries(
getSharedLabelColor().getColorMap(),
);
+ // an empty color scheme should not bring shared label colors forward
+ const sharedLabelColors = currentColorScheme
+ ? currentSharedLabelColors
+ : {};
const data = {
certified_by: dashboardInfo.certified_by,
@@ -395,7 +399,7 @@ class Header extends React.PureComponent {
color_scheme: currentColorScheme,
positions,
refresh_frequency: refreshFrequency,
- shared_label_colors: currentSharedLabelColors,
+ shared_label_colors: sharedLabelColors,
},
};
diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
index 1b5ef5fceb..d1ebc3d84e 100644
--- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
+++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx
@@ -381,16 +381,21 @@ const PropertiesModal = ({
const sharedLabelColor = getSharedLabelColor();
const categoricalNamespace =
CategoricalColorNamespace.getNamespace(colorNamespace);
+
+ // reset forced colors
categoricalNamespace.resetColors();
+
if (currentColorScheme) {
+ // reset the shared label color map based on the current color scheme
sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme);
+ // store the shared label color map and domain in the metadata
metadata.shared_label_colors = Object.fromEntries(
sharedLabelColor.getColorMap(),
);
metadata.color_scheme_domain =
categoricalSchemeRegistry.get(colorScheme)?.colors || [];
} else {
- sharedLabelColor.reset();
+ sharedLabelColor.clear();
metadata.shared_label_colors = {};
metadata.color_scheme_domain = [];
}
diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
index c9f35d3ba5..fc0bf5aabd 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx
@@ -18,11 +18,19 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
-import { styled, t } from '@superset-ui/core';
+import {
+ getSharedLabelColor,
+ styled,
+ SupersetClient,
+ t,
+} from '@superset-ui/core';
import { connect } from 'react-redux';
import { LineEditableTabs } from 'src/components/Tabs';
import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils';
import { AntdModal } from 'src/components';
+import { isEqual } from 'lodash';
+import jsonStringify from 'json-stringify-pretty-compact';
+import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo';
import { Draggable } from '../dnd/DragDroppable';
import DragHandle from '../dnd/DragHandle';
import DashboardComponent from '../../containers/DashboardComponent';
@@ -256,10 +264,51 @@ export class Tabs extends React.PureComponent {
}
};
+ // the initial color map is generated on save and catches all rendered charts
+ // charts hidden in nested tabs are not rendered, thus not included
+ // must update the label color map when switching tabs to include these charts
+ updateLabelColorsMap() {
+ const currentLabelColorsMap = Object.fromEntries(
+ getSharedLabelColor().getColorMap(),
+ );
+ const { metadata } = this.props.dashboardInfo;
+ const labelColorsMap = metadata?.shared_label_colors || {};
+ // color consistency is currently only supported when a color scheme is set
+ if (
+ metadata?.color_scheme &&
+ !isEqual(labelColorsMap, currentLabelColorsMap)
+ ) {
+ const updatedMetadata = {
+ ...metadata,
+ shared_label_colors: currentLabelColorsMap,
+ };
+ SupersetClient.put({
+ endpoint: `/api/v1/dashboard/${this.props.dashboardId}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({
+ json_metadata: jsonStringify(updatedMetadata),
+ }),
+ })
+ .then(() =>
+ this.props.dispatch(
+ dashboardInfoChanged({
+ metadata: updatedMetadata,
+ }),
+ ),
+ )
+ .catch(e => console.error(e));
+ }
+ }
+
handleClickTab(tabIndex) {
const { component } = this.props;
const { children: tabIds } = component;
+ // the charts need to render first
+ setTimeout(() => {
+ this.updateLabelColorsMap();
+ }, 500);
+
if (tabIndex !== this.state.tabIndex) {
const pathToTabIndex = getDirectPathToTabIndex(component, tabIndex);
const targetTabId = pathToTabIndex[pathToTabIndex.length - 1];
diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
index 68478adb07..f600463ee9 100644
--- a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
+++ b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx
@@ -78,6 +78,7 @@ function mapStateToProps(
editMode: dashboardState.editMode,
filters: getActiveFilters(),
dashboardId: dashboardInfo.id,
+ dashboardInfo,
fullSizeChartId: dashboardState.fullSizeChartId,
};
diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
index c01dc76f1e..d1bb7c2ac4 100644
--- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx
+++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx
@@ -188,6 +188,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => {
useEffect(() => {
const sharedLabelColor = getSharedLabelColor();
sharedLabelColor.source = SharedLabelColorSource.Dashboard;
+
return () => {
// clean up label color
const categoricalNamespace = CategoricalColorNamespace.getNamespace(
diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
index dba78af035..d7c1a804a9 100644
--- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
@@ -24,6 +24,7 @@ import { Tooltip } from 'src/components/Tooltip';
import {
CategoricalColorNamespace,
css,
+ getSharedLabelColor,
logging,
SupersetClient,
t,
@@ -95,6 +96,16 @@ export const ExploreChartHeader = ({
const { dashboards } = metadata || {};
const dashboard =
dashboardId && dashboards && dashboards.find(d => d.id === dashboardId);
+ const categoricalNamespace = CategoricalColorNamespace.getNamespace();
+ const sharedLabelColor = getSharedLabelColor();
+
+ if (!dashboard) {
+ // clean up color namespace and shared color maps
+ // to avoid colors spill outside of dashboard context
+ categoricalNamespace.resetColors();
+ sharedLabelColor.clear();
+ return;
+ }
if (dashboard) {
try {
@@ -106,21 +117,19 @@ export const ExploreChartHeader = ({
const result = response?.json?.result;
// setting the chart to use the dashboard custom label colors if any
- const metadata = JSON.parse(result.json_metadata);
- const sharedLabelColors = metadata.shared_label_colors || {};
- const customLabelColors = metadata.label_colors || {};
+ const dashboardMetadata = JSON.parse(result.json_metadata);
+ const sharedLabelColors = dashboardMetadata.shared_label_colors || {};
+ const customLabelColors = dashboardMetadata.label_colors || {};
const mergedLabelColors = {
...sharedLabelColors,
...customLabelColors,
};
- const categoricalNamespace = CategoricalColorNamespace.getNamespace();
-
Object.keys(mergedLabelColors).forEach(label => {
categoricalNamespace.setColor(
label,
mergedLabelColors[label],
- metadata.color_scheme,
+ dashboardMetadata.color_scheme,
);
});
} catch (error) {
@@ -130,7 +139,7 @@ export const ExploreChartHeader = ({
};
useEffect(() => {
- if (dashboardId) updateCategoricalNamespace();
+ updateCategoricalNamespace();
}, []);
const openPropertiesModal = () => {
diff --git a/tests/integration_tests/superset_test_config.py b/tests/integration_tests/superset_test_config.py
index b0461e79fc..977eb1c6b5 100644
--- a/tests/integration_tests/superset_test_config.py
+++ b/tests/integration_tests/superset_test_config.py
@@ -69,6 +69,7 @@ FEATURE_FLAGS = {
"SHARE_QUERIES_VIA_KV_STORE": True,
"ENABLE_TEMPLATE_PROCESSING": True,
"ALERT_REPORTS": True,
+ "AVOID_COLORS_COLLISION": True,
"DRILL_TO_DETAIL": True,
"DRILL_BY": True,
"HORIZONTAL_FILTER_BAR": True,