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 8cefe6c9d9..97cef2cae2 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -127,7 +127,7 @@ function selectColorScheme(color: string) { } function applyChanges() { - cy.getBySel('properties-modal-apply-button').click(); + cy.getBySel('properties-modal-apply-button').click({ force: true }); } function saveChanges() { @@ -209,7 +209,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(31, 168, 201)'); + .should('have.css', 'fill', 'rgb(69, 78, 124)'); }); it('should apply same color to same labels with color scheme set', () => { @@ -230,7 +230,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); // open 2nd main tab openTab(0, 1); @@ -239,7 +239,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); }); it('should apply same color to same labels with no color scheme set', () => { @@ -260,7 +260,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(31, 168, 201)'); + .should('have.css', 'fill', 'rgb(69, 78, 124)'); // open 2nd main tab openTab(0, 1); @@ -269,7 +269,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(31, 168, 201)'); + .should('have.css', 'fill', 'rgb(69, 78, 124)'); }); it('custom label colors should take the precedence in nested tabs', () => { @@ -383,17 +383,17 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .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(69, 78, 124)'); 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)'); + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(90, 193, 137)'); + .should('have.css', 'fill', 'rgb(168, 104, 183)'); openProperties(); cy.get('[aria-label="Select color scheme"]').should('have.value', ''); @@ -422,17 +422,17 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .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(69, 78, 124)'); 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)'); + cy.get( + '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(90, 193, 137)'); + .should('have.css', 'fill', 'rgb(168, 104, 183)'); }); it('should show the same colors in Explore', () => { @@ -463,7 +463,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(1) - .should('have.css', 'fill', 'rgb(108, 131, 142)'); + .should('have.css', 'fill', 'rgb(157, 172, 185)'); openExplore('Top 10 California Names Timeseries'); @@ -495,7 +495,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); // open 2nd main tab openTab(0, 1); @@ -504,7 +504,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); editDashboard(); openProperties(); @@ -569,7 +569,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(255, 90, 95)'); + .should('have.css', 'fill', 'rgb(140, 224, 113)'); // go back to first tab openTab(0, 0); @@ -616,7 +616,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); // 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 3fc093d559..7a59372ad3 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -73,15 +73,33 @@ class CategoricalColorScale extends ExtensibleFunction { this.multiple = 0; } + removeSharedLabelColorFromRange( + sharedColorMap: Map, + cleanedValue: string, + ) { + // make sure we don't overwrite the origin colors + const updatedRange = [...this.originColors]; + // remove the color option from shared color + sharedColorMap.forEach((value: string, key: string) => { + if (key !== cleanedValue) { + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); + } + }); + this.range(updatedRange.length > 0 ? updatedRange : this.originColors); + } + getColor(value?: string, sliceId?: number) { const cleanedValue = stringifyAndTrim(value); const sharedLabelColor = getSharedLabelColor(); + const sharedColorMap = sharedLabelColor.getColorMap(); + const sharedColor = sharedColorMap.get(cleanedValue); // priority: parentForcedColors > forcedColors > labelColors let color = this.parentForcedColors?.[cleanedValue] || this.forcedColors?.[cleanedValue] || - sharedLabelColor.getColorMap().get(cleanedValue); + sharedColor; if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { const multiple = Math.floor( @@ -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); + } } + sharedLabelColor.addSlice(cleanedValue, color, sliceId); return color; diff --git a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts index 2cf67b080c..48e54d1841 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts +++ b/superset-frontend/packages/superset-ui-core/src/utils/featureFlags.ts @@ -59,6 +59,7 @@ export enum FeatureFlag { TAGGING_SYSTEM = 'TAGGING_SYSTEM', VERSIONED_EXPORT = 'VERSIONED_EXPORT', SSH_TUNNELING = 'SSH_TUNNELING', + AVOID_COLORS_COLLISION = 'AVOID_COLORS_COLLISION', } export type ScheduleQueriesProps = { JSONSCHEMA: { 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 df79e778b6..0a09df6609 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,7 +18,11 @@ */ import { ScaleOrdinal } from 'd3-scale'; -import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core'; +import { + CategoricalColorScale, + FeatureFlag, + getSharedLabelColor, +} from '@superset-ui/core'; describe('CategoricalColorScale', () => { it('exists', () => { @@ -116,6 +120,24 @@ describe('CategoricalColorScale', () => { scale.getColor('goat'); expect(scale.range()).toHaveLength(6); }); + + it('should remove shared color from range if avoid colors collision enabled', () => { + window.featureFlags = { + [FeatureFlag.AVOID_COLORS_COLLISION]: 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.AVOID_COLORS_COLLISION]: false, + }; }); describe('.setColor(value, forcedColor)', () => { it('overrides default color', () => { @@ -222,4 +244,34 @@ describe('CategoricalColorScale', () => { expect(scale('pig')).toBe('blue'); }); }); + + 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']); + + 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(); + }); + + 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(); + }); + }); }); diff --git a/superset/config.py b/superset/config.py index 014d7d24bd..6ad1584601 100644 --- a/superset/config.py +++ b/superset/config.py @@ -495,6 +495,7 @@ DEFAULT_FEATURE_FLAGS: Dict[str, bool] = { # Users must check whether the DB engine supports SSH Tunnels # otherwise enabling this flag won't have any effect on the DB. "SSH_TUNNELING": False, + "AVOID_COLORS_COLLISION": True, } # ------------------------------