From 443f913395be31c7efd2d54c8f9034f34d5bf494 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 20 Apr 2023 15:24:35 -0700 Subject: [PATCH 1/9] fix: avoid color collision in chart --- .../src/color/CategoricalColorScale.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) 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 3fc093d559a2a..4a3572bc4a083 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -76,12 +76,13 @@ class CategoricalColorScale extends ExtensibleFunction { getColor(value?: string, sliceId?: number) { const cleanedValue = stringifyAndTrim(value); const sharedLabelColor = getSharedLabelColor(); + const sharedColorMap = sharedLabelColor.getColorMap(); // priority: parentForcedColors > forcedColors > labelColors let color = this.parentForcedColors?.[cleanedValue] || this.forcedColors?.[cleanedValue] || - sharedLabelColor.getColorMap().get(cleanedValue); + sharedColorMap.get(cleanedValue); if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { const multiple = Math.floor( @@ -96,6 +97,17 @@ class CategoricalColorScale extends ExtensibleFunction { const newColor = this.scale(cleanedValue); if (!color) { color = newColor; + if (sharedColorMap.size > 0) { + const updatedRange = [...this.originColors]; + sharedColorMap.forEach((value, key) => { + if (key !== cleanedValue) { + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); + } + }); + this.range(updatedRange); + color = this.scale(cleanedValue); + } } sharedLabelColor.addSlice(cleanedValue, color, sliceId); From fea753ab0507060d821cc997dc395cc19d38711c Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 20 Apr 2023 15:54:51 -0700 Subject: [PATCH 2/9] added comments --- .../src/color/CategoricalColorScale.ts | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) 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 4a3572bc4a083..593793a1517c4 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -77,12 +77,13 @@ class CategoricalColorScale extends ExtensibleFunction { 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] || - sharedColorMap.get(cleanedValue); + sharedColor; if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { const multiple = Math.floor( @@ -97,18 +98,24 @@ class CategoricalColorScale extends ExtensibleFunction { const newColor = this.scale(cleanedValue); if (!color) { color = newColor; - if (sharedColorMap.size > 0) { - const updatedRange = [...this.originColors]; - sharedColorMap.forEach((value, key) => { - if (key !== cleanedValue) { - const index = updatedRange.indexOf(value); - updatedRange.splice(index, 1); - } - }); - this.range(updatedRange); - color = this.scale(cleanedValue); - } } + + if (sharedColorMap.size > 0 && !sharedColor) { + // make sure we don't overwrite the origin colors + const updatedRange = [...this.originColors]; + + // remove the color option from shared color + sharedColorMap.forEach((value, key) => { + if (key !== cleanedValue) { + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); + } + }); + + this.range(updatedRange); + color = this.scale(cleanedValue); + } + sharedLabelColor.addSlice(cleanedValue, color, sliceId); return color; From 2d0f564c9c5486f0db9a4688d24f7cfa00eb4604 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 20 Apr 2023 20:29:44 -0700 Subject: [PATCH 3/9] repeated color when there is none --- .../src/color/CategoricalColorScale.ts | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) 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 593793a1517c4..5350680c11c12 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -98,22 +98,20 @@ class CategoricalColorScale extends ExtensibleFunction { const newColor = this.scale(cleanedValue); if (!color) { color = newColor; - } - - if (sharedColorMap.size > 0 && !sharedColor) { - // make sure we don't overwrite the origin colors - const updatedRange = [...this.originColors]; - - // remove the color option from shared color - sharedColorMap.forEach((value, key) => { - if (key !== cleanedValue) { - const index = updatedRange.indexOf(value); - updatedRange.splice(index, 1); - } - }); - - this.range(updatedRange); - color = this.scale(cleanedValue); + if (!sharedColor) { + // make sure we don't overwrite the origin colors + const updatedRange = [...this.originColors]; + // remove the color option from shared color + sharedColorMap.forEach((value, key) => { + if (key !== cleanedValue) { + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); + } + }); + + this.range(updatedRange.length > 0 ? updatedRange : this.originColors); + color = this.scale(cleanedValue); + } } sharedLabelColor.addSlice(cleanedValue, color, sliceId); From 8c7fc749213a2a0ffef7d6966a25a50219227bbb Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Tue, 25 Apr 2023 14:20:50 -0700 Subject: [PATCH 4/9] fix tests --- .../superset-ui-core/src/color/CategoricalColorScale.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 5350680c11c12..609875cf2901c 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -98,8 +98,8 @@ class CategoricalColorScale extends ExtensibleFunction { const newColor = this.scale(cleanedValue); if (!color) { color = newColor; - if (!sharedColor) { - // make sure we don't overwrite the origin colors + // make sure we don't overwrite the origin colors + if (!isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { const updatedRange = [...this.originColors]; // remove the color option from shared color sharedColorMap.forEach((value, key) => { From bd4679d24be3a78edf72ffe914db663f09030876 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Wed, 26 Apr 2023 16:50:59 -0700 Subject: [PATCH 5/9] update color consistency e2e tests --- .../cypress/e2e/dashboard/editmode.test.ts | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) 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 8cefe6c9d9756..dfe492bf4e83d 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -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', () => { @@ -359,17 +359,17 @@ 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(255, 0, 0)'); cy.get( '[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)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(41, 171, 226)'); + .should('have.css', 'fill', 'rgb(45, 85, 132)'); }); it('should re-apply original color after removing custom label color with no color scheme set', () => { @@ -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)'); + .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(69, 78, 124)'); + .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', ''); @@ -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); @@ -592,7 +592,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(140, 224, 113)'); }); it('should apply the color scheme in nested tabs', () => { @@ -616,14 +616,14 @@ 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); waitForChartLoad({ name: 'Growth Rate', viz: 'line' }); cy.get('[data-test-chart-name="Growth Rate"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(234, 11, 140)'); + .should('have.css', 'fill', 'rgb(51, 217, 193)'); }); it('should apply a valid color scheme for rendered charts in nested tabs', () => { From 844bee8ebad7a1af8b6be66d389261e2175c5f72 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Wed, 26 Apr 2023 17:05:39 -0700 Subject: [PATCH 6/9] update e2e --- .../cypress-base/cypress/e2e/dashboard/editmode.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dfe492bf4e83d..3088c6c4e9fcb 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -623,7 +623,7 @@ describe('Dashboard edit', () => { waitForChartLoad({ name: 'Growth Rate', viz: 'line' }); cy.get('[data-test-chart-name="Growth Rate"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(51, 217, 193)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); }); it('should apply a valid color scheme for rendered charts in nested tabs', () => { From 2c6e38fb22d5962065afb2e06ce229424a9e95b5 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 27 Apr 2023 08:55:24 -0700 Subject: [PATCH 7/9] fix e2e --- .../cypress-base/cypress/e2e/dashboard/editmode.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3088c6c4e9fcb..48b23ce18d639 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -592,7 +592,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(140, 224, 113)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); }); it('should apply the color scheme in nested tabs', () => { From f978b1fccce70fc14a9572f70b595ea4ffaeab48 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Fri, 12 May 2023 22:45:01 -0700 Subject: [PATCH 8/9] add test --- .../src/color/CategoricalColorScale.ts | 33 ++++++----- .../test/color/CategoricalColorScale.test.ts | 55 ++++++++++++++++++- 2 files changed, 74 insertions(+), 14 deletions(-) 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 609875cf2901c..f9d0056deeae2 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -73,6 +73,24 @@ class CategoricalColorScale extends ExtensibleFunction { this.multiple = 0; } + removeSharedLabelColorFromRange( + sharedColorMap: Map, + cleanedValue: string, + ) { + if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { + return; + } + + const updatedRange = [...this.originColors]; + // remove the color option from shared color + sharedColorMap.forEach((value: string, key: string) => { + if (key !== cleanedValue) { + updatedRange.splice(updatedRange.indexOf(value), 1); + } + }); + this.range(updatedRange.length > 0 ? updatedRange : this.originColors); + } + getColor(value?: string, sliceId?: number) { const cleanedValue = stringifyAndTrim(value); const sharedLabelColor = getSharedLabelColor(); @@ -99,19 +117,8 @@ class CategoricalColorScale extends ExtensibleFunction { if (!color) { color = newColor; // make sure we don't overwrite the origin colors - if (!isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { - const updatedRange = [...this.originColors]; - // remove the color option from shared color - sharedColorMap.forEach((value, key) => { - if (key !== cleanedValue) { - const index = updatedRange.indexOf(value); - updatedRange.splice(index, 1); - } - }); - - this.range(updatedRange.length > 0 ? updatedRange : this.originColors); - color = this.scale(cleanedValue); - } + this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue); + color = this.scale(cleanedValue); } sharedLabelColor.addSlice(cleanedValue, color, sliceId); 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 df79e778b6b75..8b39b9644f178 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', () => { @@ -222,4 +226,53 @@ describe('CategoricalColorScale', () => { expect(scale('pig')).toBe('blue'); }); }); + + describe('.removeSharedLabelColorFromRange(colorMap, cleanedValue)', () => { + it('range should be the same if analogous colors enabled', () => { + window.featureFlags = { + [FeatureFlag.USE_ANALAGOUS_COLORS]: true, + }; + const colors = ['blue', 'green', 'red']; + const scale = new CategoricalColorScale(colors); + expect(scale.range()).toEqual(colors); + + const sharedLabelColor = getSharedLabelColor(); + const colorMap = sharedLabelColor.getColorMap(); + sharedLabelColor.addSlice('cow', 'blue', 1); + sharedLabelColor.addSlice('goat', 'green', 1); + scale.removeSharedLabelColorFromRange(colorMap, 'pig'); + expect(scale.range()).toEqual(colors); + sharedLabelColor.clear(); + }); + + it('should remove shared color from range', () => { + window.featureFlags = { + [FeatureFlag.USE_ANALAGOUS_COLORS]: false, + }; + 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); + 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(); + }); + }); }); From d5d68b7a20bb87425a46d86fd254180e9280bcb9 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Mon, 15 May 2023 17:19:19 -0700 Subject: [PATCH 9/9] add feature flag AVOID_COLORS_COLLISION and update e2e tests --- .../cypress/e2e/dashboard/editmode.test.ts | 14 +++---- .../src/color/CategoricalColorScale.ts | 15 ++++--- .../src/utils/featureFlags.ts | 1 + .../test/color/CategoricalColorScale.test.ts | 39 +++++++++---------- superset/config.py | 1 + 5 files changed, 35 insertions(+), 35 deletions(-) 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 48b23ce18d639..97cef2cae2f5c 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() { @@ -359,17 +359,17 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(255, 0, 0)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(1) - .should('have.css', 'fill', 'rgb(157, 172, 185)'); + .should('have.css', 'fill', 'rgb(108, 131, 142)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(45, 85, 132)'); + .should('have.css', 'fill', 'rgb(41, 171, 226)'); }); it('should re-apply original color after removing custom label color with no color scheme set', () => { @@ -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)'); + .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(69, 78, 124)'); + .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', () => { 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 f9d0056deeae2..7a59372ad3af0 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -77,15 +77,13 @@ class CategoricalColorScale extends ExtensibleFunction { sharedColorMap: Map, cleanedValue: string, ) { - if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { - return; - } - + // 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) { - updatedRange.splice(updatedRange.indexOf(value), 1); + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); } }); this.range(updatedRange.length > 0 ? updatedRange : this.originColors); @@ -116,9 +114,10 @@ class CategoricalColorScale extends ExtensibleFunction { const newColor = this.scale(cleanedValue); if (!color) { color = newColor; - // make sure we don't overwrite the origin colors - this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue); - color = this.scale(cleanedValue); + if (isFeatureEnabled(FeatureFlag.AVOID_COLORS_COLLISION)) { + this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue); + color = this.scale(cleanedValue); + } } sharedLabelColor.addSlice(cleanedValue, color, sliceId); 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 2cf67b080c68a..48e54d18416a8 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 8b39b9644f178..0a09df66091cb 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 @@ -120,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', () => { @@ -228,31 +246,12 @@ describe('CategoricalColorScale', () => { }); describe('.removeSharedLabelColorFromRange(colorMap, cleanedValue)', () => { - it('range should be the same if analogous colors enabled', () => { - window.featureFlags = { - [FeatureFlag.USE_ANALAGOUS_COLORS]: true, - }; - const colors = ['blue', 'green', 'red']; - const scale = new CategoricalColorScale(colors); - expect(scale.range()).toEqual(colors); - - const sharedLabelColor = getSharedLabelColor(); - const colorMap = sharedLabelColor.getColorMap(); - sharedLabelColor.addSlice('cow', 'blue', 1); - sharedLabelColor.addSlice('goat', 'green', 1); - scale.removeSharedLabelColorFromRange(colorMap, 'pig'); - expect(scale.range()).toEqual(colors); - sharedLabelColor.clear(); - }); - it('should remove shared color from range', () => { - window.featureFlags = { - [FeatureFlag.USE_ANALAGOUS_COLORS]: false, - }; 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'); diff --git a/superset/config.py b/superset/config.py index ea602338d3aa1..998edf6b47daf 100644 --- a/superset/config.py +++ b/superset/config.py @@ -496,6 +496,7 @@ class D3Format(TypedDict, total=False): # 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, } # ------------------------------