add feature flag AVOID_COLORS_COLLISION and update e2e tests

apache · May 16, 2023 · d5d68b7 · d5d68b7
1 parent f978b1f
commit d5d68b7
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 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
@@ -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
@@ -77,15 +77,13 @@ class CategoricalColorScale extends ExtensibleFunction {
     sharedColorMap: Map<string, string>,
     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
@@ -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
@@ -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
@@ -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,
 }
 
 # ------------------------------