chore(Dashboard): Clean up color consistency functions

apache · Nov 21, 2024 · d78af29 · d78af29
1 parent 69cd840
commit d78af29
Show file tree

Hide file tree

Showing 15 changed files with 291 additions and 147 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
@@ -165,27 +165,23 @@ function selectColorScheme(
     .first()
     .then($input => {
       cy.wrap($input).click({ force: true });
-      cy.wrap($input).type(color.slice(0, 3), { force: true });
+      cy.wrap($input).type(color.slice(0, 5), { force: true });
     });
   cy.getBySel(color).click({ force: true });
 }
 
-function setExploreAdmin(username: string) {
+function saveAndGo(dashboard = 'Tabbed Dashboard') {
   interceptExploreUpdate();
-  cy.get(`input[aria-label="Owners"]`)
-    .first()
-    .then($input => {
-      cy.get('body').then($body => {
-        if ($body.find('.ant-tag-close-icon').length) {
-          cy.get('.ant-tag-close-icon').click({ force: true, multiple: true });
-        }
-        cy.wrap($input).click({ force: true });
-        cy.wrap($input).type(username.slice(0, 5), { force: true });
-        cy.wrap($input).type('{enter}');
-      });
-    });
-  cy.getBySel('properties-modal-save-button').click();
-  cy.wait('@chartUpdate');
+  cy.getBySel('query-save-button').click();
+  cy.getBySel('save-modal-body').then($modal => {
+    cy.wrap($modal)
+      .find("div[aria-label='Select a dashboard'] .ant-select-selection-item")
+      .should('have.text', dashboard);
+    cy.getBySel('save-overwrite-radio').should('not.be.disabled');
+    cy.getBySel('save-overwrite-radio').click();
+    cy.get('#btn_modal_save_goto_dash').click();
+    cy.wait('@chartUpdate');
+  });
 }
 
 function applyChanges() {
@@ -228,7 +224,7 @@ function writeMetadata(metadata: string) {
   });
 }
 
-function openExplore(chartName: string) {
+function openExploreWithDashboardContext(chartName: string) {
   interceptExploreJson();
   interceptGet();
 
@@ -245,11 +241,24 @@ function openExplore(chartName: string) {
   cy.get('.chart-container').should('exist');
 }
 
+function saveExploreColorScheme(
+  chart = 'Top 10 California Names Timeseries',
+  colorScheme = 'supersetColors',
+) {
+  interceptExploreUpdate();
+  openExploreWithDashboardContext(chart);
+  openTab(0, 1, 'control-tabs');
+  selectColorScheme(colorScheme, 'control-item');
+  cy.getBySel('query-save-button').click();
+  cy.getBySel('save-overwrite-radio').click();
+  cy.getBySel('btn-modal-save').click();
+  cy.wait('@chartUpdate');
+}
+
 describe('Dashboard edit', () => {
   describe('Color consistency', () => {
     beforeEach(() => {
       resetDashboardColors();
-      resetDashboardColors('supported_charts_dash');
     });
 
     it('should not allow to change color scheme of a chart when dashboard has one', () => {
@@ -266,7 +275,7 @@ describe('Dashboard edit', () => {
         viz: 'line',
       });
 
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -296,7 +305,7 @@ describe('Dashboard edit', () => {
       });
 
       openTab(0, 0);
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -331,7 +340,7 @@ describe('Dashboard edit', () => {
         .first()
         .should('have.css', 'fill', 'rgb(255, 0, 0)');
 
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -350,6 +359,16 @@ describe('Dashboard edit', () => {
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
         .eq(1)
         .should('have.css', 'fill', 'rgb(50, 0, 167)');
+
+      // label Daniel
+      cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
+        .eq(2)
+        .should('have.css', 'fill', 'rgb(0, 76, 218)');
+
+      // label David
+      cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
+        .eq(3)
+        .should('have.css', 'fill', 'rgb(0, 116, 241)');
     });
 
     it('should allow to change color scheme of a chart when dashboard has no scheme and show the change', () => {
@@ -369,7 +388,7 @@ describe('Dashboard edit', () => {
         .first()
         .should('have.css', 'fill', 'rgb(31, 168, 201)');
 
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -384,24 +403,15 @@ describe('Dashboard edit', () => {
         .first()
         .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
-      openExploreProperties();
-      setExploreAdmin('admin user');
-      cy.getBySel('query-save-button').click();
-      cy.getBySel('save-overwrite-radio').click();
-      cy.get('#btn_modal_save_goto_dash').click();
+      saveAndGo();
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
         .first()
         .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       // reset original scheme
-      openExplore('Top 10 California Names Timeseries');
-      openTab(0, 1, 'control-tabs');
-      selectColorScheme('supersetColors', 'control-item');
-      cy.getBySel('query-save-button').click();
-      cy.getBySel('save-overwrite-radio').click();
-      cy.get('#btn_modal_save_goto_dash').click();
+      saveExploreColorScheme();
     });
 
     it('should allow to change color scheme of a chart when dashboard has no scheme but custom label colors and show the change', () => {
@@ -427,7 +437,7 @@ describe('Dashboard edit', () => {
         .first()
         .should('have.css', 'fill', 'rgb(255, 0, 0)');
 
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -447,11 +457,7 @@ describe('Dashboard edit', () => {
         .eq(1)
         .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
-      openExploreProperties();
-      setExploreAdmin('admin user');
-      cy.getBySel('query-save-button').click();
-      cy.getBySel('save-overwrite-radio').click();
-      cy.get('#btn_modal_save_goto_dash').click();
+      saveAndGo();
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -464,12 +470,7 @@ describe('Dashboard edit', () => {
         .should('have.css', 'fill', 'rgb(50, 0, 167)');
 
       // reset original scheme
-      openExplore('Top 10 California Names Timeseries');
-      openTab(0, 1, 'control-tabs');
-      selectColorScheme('supersetColors', 'control-item');
-      cy.getBySel('query-save-button').click();
-      cy.getBySel('save-overwrite-radio').click();
-      cy.get('#btn_modal_save_goto_dash').click();
+      saveExploreColorScheme();
     });
 
     it('should not change colors on refreshes with no color scheme set', () => {
@@ -897,7 +898,7 @@ describe('Dashboard edit', () => {
         .first()
         .should('have.css', 'fill', 'rgb(255, 0, 0)');
 
-      openExplore('Top 10 California Names Timeseries');
+      openExploreWithDashboardContext('Top 10 California Names Timeseries');
 
       // label Anthony
       cy.get('[data-test="chart-container"] .line .nv-legend-symbol')
@@ -1064,6 +1065,7 @@ describe('Dashboard edit', () => {
     beforeEach(() => {
       cy.createSampleDashboards([0]);
       openProperties();
+      selectColorScheme('supersetColors');
     });
 
     it('should accept a valid color scheme', () => {

diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/colorControls.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/colorControls.ts
@@ -25,6 +25,8 @@ export const getColorControlsProps = (state: Record<string, any>) => {
       !!dashboardId && !!state?.form_data?.dashboard_color_scheme,
     hasCustomLabelsColor:
       Object.keys(state?.form_data?.label_colors || {}).length > 0,
+    colorNamespace: state?.form_data?.color_namespace,
+    mapLabelsColors: state?.form_data?.map_label_colors || {},
     sharedLabelsColors: state?.form_data?.shared_label_colors || [],
   };
 };
diff --git a/superset-frontend/packages/superset-ui-chart-controls/test/utils/colorControls.test.ts b/superset-frontend/packages/superset-ui-chart-controls/test/utils/colorControls.test.ts
@@ -27,6 +27,8 @@ describe('getColorControlsProps', () => {
       dashboardId: undefined,
       hasDashboardColorScheme: false,
       hasCustomLabelsColor: false,
+      colorNamespace: undefined,
+      mapLabelsColors: {},
       sharedLabelsColors: [],
     });
   });
@@ -46,6 +48,8 @@ describe('getColorControlsProps', () => {
       dashboardId: 123,
       hasDashboardColorScheme: true,
       hasCustomLabelsColor: false,
+      colorNamespace: undefined,
+      mapLabelsColors: {},
       sharedLabelsColors: [],
     });
   });
@@ -64,6 +68,8 @@ describe('getColorControlsProps', () => {
       dashboardId: 123,
       hasDashboardColorScheme: false,
       hasCustomLabelsColor: true,
+      colorNamespace: undefined,
+      mapLabelsColors: {},
       sharedLabelsColors: [],
     });
   });
@@ -81,6 +87,8 @@ describe('getColorControlsProps', () => {
       hasDashboardColorScheme: false,
       hasCustomLabelsColor: false,
       sharedLabelsColors: ['#FF5733', '#33FF57'],
+      colorNamespace: undefined,
+      mapLabelsColors: {},
     });
   });
 
@@ -96,6 +104,8 @@ describe('getColorControlsProps', () => {
       dashboardId: 789,
       hasDashboardColorScheme: false,
       hasCustomLabelsColor: false,
+      colorNamespace: undefined,
+      mapLabelsColors: {},
       sharedLabelsColors: [],
     });
   });

diff --git a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts
@@ -72,7 +72,7 @@ export default class CategoricalColorNamespace {
     this.forcedItems = {};
   }
 
-  resetIndividualColors(labels: string[] = []) {
+  resetColorsForLabels(labels: string[] = []) {
     const updatedForcedItems = cloneDeep(this.forcedItems);
     labels.forEach(label => {
       if (updatedForcedItems.hasOwnProperty(label)) {

diff --git a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts
@@ -101,8 +101,8 @@ export class LabelsColorMap {
   ) {
     const chartConfig = this.chartsLabelsMap.get(sliceId) || {
       labels: [],
-      scheme: '',
-      ownScheme: '',
+      scheme: undefined,
+      ownScheme: undefined,
     };
 
     const { labels } = chartConfig;
@@ -111,6 +111,7 @@ export class LabelsColorMap {
       this.chartsLabelsMap.set(sliceId, {
         labels,
         scheme: colorScheme,
+        ownScheme: chartConfig.ownScheme,
       });
     }
     if (this.source === LabelsColorMapSource.Dashboard) {

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
@@ -161,20 +161,20 @@ describe('CategoricalColorNamespace', () => {
       expect(color).toBe(color2);
     });
   });
-  describe('statis resetIndividualColors(labels)', () => {
+  describe('statis resetColorsForLabels(labels)', () => {
     it('removes specified labels from forcedItems', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
       namespace.setColor('label2', 'blue');
-      namespace.resetIndividualColors(['label1']);
+      namespace.resetColorsForLabels(['label1']);
 
       expect(namespace.forcedItems).toMatchObject({ label2: 'blue' });
     });
     it('does not modify forcedItems if no labels are provided', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
       namespace.setColor('label2', 'blue');
-      namespace.resetIndividualColors();
+      namespace.resetColorsForLabels();
 
       expect(namespace.forcedItems).toMatchObject({
         label1: 'red',
@@ -184,15 +184,15 @@ describe('CategoricalColorNamespace', () => {
     it('does nothing if the label is not in forcedItems', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
-      namespace.resetIndividualColors(['label2']); // label2 doesn't exist
+      namespace.resetColorsForLabels(['label2']); // label2 doesn't exist
 
       expect(namespace.forcedItems).toMatchObject({ label1: 'red' });
     });
     it('removes all labels when all are provided', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
       namespace.setColor('label2', 'blue');
-      namespace.resetIndividualColors(['label1', 'label2']);
+      namespace.resetColorsForLabels(['label1', 'label2']);
 
       expect(namespace.forcedItems).toMatchObject({});
     });
@@ -201,21 +201,21 @@ describe('CategoricalColorNamespace', () => {
       namespace.setColor('label1', 'red');
 
       const originalForcedItems = namespace.forcedItems;
-      namespace.resetIndividualColors(['label1']);
+      namespace.resetColorsForLabels(['label1']);
 
       expect(originalForcedItems).not.toBe(namespace.forcedItems);
     });
     it('removes the label if it exists in updatedForcedItems', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
-      namespace.resetIndividualColors(['label1']);
+      namespace.resetColorsForLabels(['label1']);
 
       expect(namespace.forcedItems).toEqual({});
     });
     it('does nothing for a label not in updatedForcedItems', () => {
       const namespace = getNamespace('test-reset-individual');
       namespace.setColor('label1', 'red');
-      namespace.resetIndividualColors(['label2']); // label2 doesn't exist
+      namespace.resetColorsForLabels(['label2']); // label2 doesn't exist
 
       expect(namespace.forcedItems).toEqual({ label1: 'red' });
     });

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
@@ -33,7 +33,7 @@ describe('CategoricalColorScale', () => {
     expect(CategoricalColorScale !== undefined).toBe(true);
   });
 
-  describe('new CategoricalColorScale(colors, forcedColors, colorScheme, originalScheme)', () => {
+  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);