From 5eeb5ebb1b7752165ff4b03594ebd3009efd9c18 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Tue, 11 Jul 2023 16:12:10 -0700 Subject: [PATCH 1/8] fix: color collision with tabs --- .../superset-ui-core/src/color/CategoricalColorScale.ts | 6 ++++++ 1 file changed, 6 insertions(+) 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 7a59372ad3af0..dc527e216be89 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -86,6 +86,12 @@ class CategoricalColorScale extends ExtensibleFunction { updatedRange.splice(index, 1); } }); + Object.entries(this.parentForcedColors).forEach(([key, value]) => { + if (key !== cleanedValue) { + const index = updatedRange.indexOf(value); + updatedRange.splice(index, 1); + } + }); // remove the color option from forced colors this.range(updatedRange.length > 0 ? updatedRange : this.originColors); } From 0398aafe8ed5eddcb025df0599b69038bf10ee09 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Tue, 11 Jul 2023 18:16:28 -0700 Subject: [PATCH 2/8] fix tests --- .../cypress/e2e/dashboard/editmode.test.ts | 10 +++++----- .../test/color/CategoricalColorScale.test.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 5 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 416af35182288..9c34534eb4643 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -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(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); // open 2nd main tab openTab(0, 1); @@ -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(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(108, 131, 142)'); 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(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); // open 2nd main tab openTab(0, 1); @@ -541,7 +541,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', () => { @@ -587,7 +587,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/test/color/CategoricalColorScale.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts index 0a09df66091cb..0d98198de13b7 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 @@ -273,5 +273,21 @@ describe('CategoricalColorScale', () => { expect(scale.range()).toEqual(['blue', 'green', 'red']); sharedLabelColor.clear(); }); + + 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(); + }); }); }); From b7eef8c5be2aadb08ff91b31fd4fea69315577ee Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Tue, 11 Jul 2023 22:36:00 -0700 Subject: [PATCH 3/8] fix e2e tests --- .../cypress-base/cypress/e2e/dashboard/editmode.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 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 9c34534eb4643..3f70f4e89b3a2 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -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(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); }); it('should apply same color to same labels with no color scheme set', () => { @@ -474,7 +474,7 @@ describe('Dashboard edit', () => { // label Christopher cy.get('[data-test="chart-container"] .line .nv-legend-symbol') .eq(1) - .should('have.css', 'fill', 'rgb(108, 131, 142)'); + .should('have.css', 'fill', 'rgb(51, 61, 71)'); }); it('should change color scheme multiple times', () => { @@ -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(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); editDashboard(); openProperties(); From 04c0aab562870301eb16540614b976d88c1655b2 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Wed, 12 Jul 2023 13:06:08 -0700 Subject: [PATCH 4/8] use filter --- .../superset-ui-core/src/color/CategoricalColorScale.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 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 dc527e216be89..a9cd9bd6a418a 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -78,18 +78,16 @@ class CategoricalColorScale extends ExtensibleFunction { cleanedValue: string, ) { // make sure we don't overwrite the origin colors - const updatedRange = [...this.originColors]; + let 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); + updatedRange = updatedRange.filter(color => color !== value); } }); Object.entries(this.parentForcedColors).forEach(([key, value]) => { if (key !== cleanedValue) { - const index = updatedRange.indexOf(value); - updatedRange.splice(index, 1); + updatedRange = updatedRange.filter(color => color !== value); } }); // remove the color option from forced colors this.range(updatedRange.length > 0 ? updatedRange : this.originColors); From aec5845d6b19f30d27096e50d4e5b545e482399b Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Wed, 12 Jul 2023 13:58:56 -0700 Subject: [PATCH 5/8] fix e2e test --- .../cypress/e2e/dashboard/editmode.test.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 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 3f70f4e89b3a2..e9128ffaa9589 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -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, 61, 71)'); // 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, 61, 71)'); }); it('should apply same color to same labels with no color scheme set', () => { @@ -382,7 +382,7 @@ describe('Dashboard edit', () => { cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) - .first() + .eq(0) .should('have.css', 'fill', 'rgb(69, 78, 124)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', @@ -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(172, 32, 119)'); openExplore('Top 10 California Names Timeseries'); @@ -474,7 +474,7 @@ describe('Dashboard edit', () => { // label Christopher cy.get('[data-test="chart-container"] .line .nv-legend-symbol') .eq(1) - .should('have.css', 'fill', 'rgb(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(172, 32, 119)'); }); it('should change color scheme multiple times', () => { @@ -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, 61, 71)'); // 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, 61, 71)'); editDashboard(); openProperties(); @@ -541,7 +541,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(51, 61, 71)'); }); it('should apply the color scheme across main tabs for rendered charts', () => { @@ -587,7 +587,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, 61, 71)'); // open another nested tab openTab(2, 1); From d6e8a6a1efcfa5d49408c83afc2ab21e392f0ddc Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 13 Jul 2023 11:27:13 -0700 Subject: [PATCH 6/8] use set --- .../cypress/e2e/dashboard/editmode.test.ts | 2 +- .../src/color/CategoricalColorScale.ts | 11 ++++++----- 2 files changed, 7 insertions(+), 6 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 e9128ffaa9589..ddd024d63bf24 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -382,7 +382,7 @@ describe('Dashboard edit', () => { cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) - .eq(0) + .first() .should('have.css', 'fill', 'rgb(69, 78, 124)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', 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 a9cd9bd6a418a..5004dce2dd7cc 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -78,19 +78,20 @@ class CategoricalColorScale extends ExtensibleFunction { cleanedValue: string, ) { // make sure we don't overwrite the origin colors - let updatedRange = [...this.originColors]; + const updatedRange = new Set(this.originColors); // remove the color option from shared color sharedColorMap.forEach((value: string, key: string) => { if (key !== cleanedValue) { - updatedRange = updatedRange.filter(color => color !== value); + updatedRange.delete(value); } }); + // remove the color option from forced colors Object.entries(this.parentForcedColors).forEach(([key, value]) => { if (key !== cleanedValue) { - updatedRange = updatedRange.filter(color => color !== value); + updatedRange.delete(value); } - }); // remove the color option from forced colors - this.range(updatedRange.length > 0 ? updatedRange : this.originColors); + }); + this.range(updatedRange.size > 0 ? [...updatedRange] : this.originColors); } getColor(value?: string, sliceId?: number) { From 573f1f03d08237f82404e28b6c69fd89179d3a06 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 13 Jul 2023 12:34:58 -0700 Subject: [PATCH 7/8] add waiting --- .../cypress-base/cypress/e2e/dashboard/editmode.test.ts | 1 + 1 file changed, 1 insertion(+) 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 ddd024d63bf24..0266dd39d94e5 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -379,6 +379,7 @@ describe('Dashboard edit', () => { name: 'Top 10 California Names Timeseries', viz: 'line', }); + cy.wait(5000); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) From 622ae33b09802e8942441e2cd677cd24a943f720 Mon Sep 17 00:00:00 2001 From: Lily Kuang Date: Thu, 13 Jul 2023 18:13:27 -0700 Subject: [PATCH 8/8] fix reapply color --- .../cypress-base/cypress/e2e/dashboard/editmode.test.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 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 0266dd39d94e5..b35105a7b5911 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -379,7 +379,6 @@ describe('Dashboard edit', () => { name: 'Top 10 California Names Timeseries', viz: 'line', }); - cy.wait(5000); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) @@ -423,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(252, 199, 0)'); + .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(143, 211, 228)'); + .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(172, 225, 196)'); + .should('have.css', 'fill', 'rgb(163, 143, 121)'); }); it('should show the same colors in Explore', () => {