fix: Color consistency (#17089)

* Update label colors on the fly * Clean up * Improve getFormDataWithExtraFilters * Improve code structure * Remove labelColors from formData * Exclude label_colors from URL * Refactor color scheme implementation * Clean up * Refactor and simplify * Fix lint * Remove unnecessary ColorMapControl * Lint * Give json color scheme precedence * Add label_colors prop in metadata * Separate owners and dashboard meta requests * Remove label_colors control * bump superset-ui 0.18.19 * Fix end of file * Update tests * Fix lint * Update Cypress * Update setColorScheme method * Use Antd modal body
apache · Nov 3, 2021 · 59a6502 · 59a6502
1 parent 85a19a9
commit 59a6502
Show file tree

Hide file tree

Showing 26 changed files with 488 additions and 500 deletions.
diff --git a/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts b/superset-frontend/cypress-base/cypress/integration/dashboard/edit_properties.test.ts
@@ -24,10 +24,10 @@ import { WORLD_HEALTH_DASHBOARD } from './dashboard.helper';
 
 function selectColorScheme(color: string) {
   // open color scheme dropdown
-  cy.get('.modal-body')
-    .contains('Color Scheme')
+  cy.get('.ant-modal-body')
+    .contains('Color scheme')
     .parents('.ControlHeader')
-    .next('.Select')
+    .next('.ant-select')
     .click()
     .then($colorSelect => {
       // select a new color scheme
@@ -37,7 +37,7 @@ function selectColorScheme(color: string) {
 
 function assertMetadata(text: string) {
   const regex = new RegExp(text);
-  cy.get('.modal-body')
+  cy.get('.ant-modal-body')
     .find('#json_metadata')
     .should('be.visible')
     .then(() => {
@@ -50,12 +50,15 @@ function assertMetadata(text: string) {
 }
 
 function typeMetadata(text: string) {
-  cy.get('.modal-body').find('#json_metadata').should('be.visible').type(text);
+  cy.get('.ant-modal-body')
+    .find('#json_metadata')
+    .should('be.visible')
+    .type(text);
 }
 
 function openAdvancedProperties() {
   return cy
-    .get('.modal-body')
+    .get('.ant-modal-body')
     .contains('Advanced')
     .should('be.visible')
     .click();
@@ -94,6 +97,10 @@ describe('Dashboard edit action', () => {
       .get('[data-test="dashboard-title-input"]')
       .type(`{selectall}{backspace}${dashboardTitle}`);
 
+    cy.wait('@dashboardGet').then(() => {
+      selectColorScheme('d3Category20b');
+    });
+
     // save edit changes
     cy.get('.ant-modal-footer')
       .contains('Save')
@@ -146,7 +153,7 @@ describe('Dashboard edit action', () => {
         .click()
         .then(() => {
           // assert that modal edit window has closed
-          cy.get('.modal-body').should('not.exist');
+          cy.get('.ant-modal-body').should('not.exist');
 
           // assert color has been updated
           openDashboardEditProperties();
@@ -177,7 +184,7 @@ describe('Dashboard edit action', () => {
         .click()
         .then(() => {
           // assert that modal edit window has closed
-          cy.get('.modal-body')
+          cy.get('.ant-modal-body')
             .contains('A valid color scheme is required')
             .should('be.visible');
         });

diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json
diff --git a/superset-frontend/package.json b/superset-frontend/package.json
@@ -68,35 +68,35 @@
     "@emotion/cache": "^11.4.0",
     "@emotion/react": "^11.4.1",
     "@emotion/styled": "^11.3.0",
-    "@superset-ui/chart-controls": "^0.18.18",
-    "@superset-ui/core": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-calendar": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-chord": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-country-map": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-event-flow": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-force-directed": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-heatmap": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-histogram": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-horizon": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-map-box": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-partition": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-rose": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-sankey": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-sunburst": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-treemap": "^0.18.18",
-    "@superset-ui/legacy-plugin-chart-world-map": "^0.18.18",
-    "@superset-ui/legacy-preset-chart-big-number": "^0.18.18",
+    "@superset-ui/chart-controls": "^0.18.19",
+    "@superset-ui/core": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-calendar": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-chord": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-country-map": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-event-flow": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-force-directed": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-heatmap": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-histogram": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-horizon": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-map-box": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-paired-t-test": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-parallel-coordinates": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-partition": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-pivot-table": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-rose": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-sankey": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-sankey-loop": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-sunburst": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-treemap": "^0.18.19",
+    "@superset-ui/legacy-plugin-chart-world-map": "^0.18.19",
+    "@superset-ui/legacy-preset-chart-big-number": "^0.18.19",
     "@superset-ui/legacy-preset-chart-deckgl": "^0.4.13",
-    "@superset-ui/legacy-preset-chart-nvd3": "^0.18.18",
-    "@superset-ui/plugin-chart-echarts": "^0.18.18",
-    "@superset-ui/plugin-chart-pivot-table": "^0.18.18",
-    "@superset-ui/plugin-chart-table": "^0.18.18",
-    "@superset-ui/plugin-chart-word-cloud": "^0.18.18",
-    "@superset-ui/preset-chart-xy": "^0.18.18",
+    "@superset-ui/legacy-preset-chart-nvd3": "^0.18.19",
+    "@superset-ui/plugin-chart-echarts": "^0.18.19",
+    "@superset-ui/plugin-chart-pivot-table": "^0.18.19",
+    "@superset-ui/plugin-chart-table": "^0.18.19",
+    "@superset-ui/plugin-chart-word-cloud": "^0.18.19",
+    "@superset-ui/preset-chart-xy": "^0.18.19",
     "@vx/responsive": "^0.0.195",
     "abortcontroller-polyfill": "^1.1.9",
     "antd": "^4.9.4",

diff --git a/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx b/superset-frontend/spec/javascripts/dashboard/components/PropertiesModal_spec.jsx
@@ -94,10 +94,12 @@ describe('PropertiesModal', () => {
       describe('without metadata', () => {
         const wrapper = setup({ colorScheme: 'SUPERSET_DEFAULT' });
         const modalInstance = wrapper.find('PropertiesModal').instance();
-        it('does not update the color scheme in the metadata', () => {
+        it('updates the color scheme in the metadata', () => {
           const spy = jest.spyOn(modalInstance, 'onMetadataChange');
           modalInstance.onColorSchemeChange('SUPERSET_DEFAULT');
-          expect(spy).not.toHaveBeenCalled();
+          expect(spy).toHaveBeenCalledWith(
+            '{"something": "foo", "color_scheme": "SUPERSET_DEFAULT", "label_colors": {}}',
+          );
         });
       });
       describe('with metadata', () => {
@@ -125,10 +127,12 @@ describe('PropertiesModal', () => {
               json_metadata: '{"timed_refresh_immune_slices": []}',
             },
           });
-          it('will not update the metadata', () => {
+          it('will update the metadata', () => {
             const spy = jest.spyOn(modalInstance, 'onMetadataChange');
             modalInstance.onColorSchemeChange('SUPERSET_DEFAULT');
-            expect(spy).not.toHaveBeenCalled();
+            expect(spy).toHaveBeenCalledWith(
+              '{"something": "foo", "color_scheme": "SUPERSET_DEFAULT", "label_colors": {}}',
+            );
           });
         });
       });

diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx
@@ -44,6 +44,7 @@ const propTypes = {
   // formData contains chart's own filter parameter
   // and merged with extra filter that current dashboard applying
   formData: PropTypes.object.isRequired,
+  labelColors: PropTypes.object,
   width: PropTypes.number,
   height: PropTypes.number,
   setControlValue: PropTypes.func,

diff --git a/superset-frontend/src/chart/ChartRenderer.jsx b/superset-frontend/src/chart/ChartRenderer.jsx
@@ -29,6 +29,7 @@ const propTypes = {
   datasource: PropTypes.object,
   initialValues: PropTypes.object,
   formData: PropTypes.object.isRequired,
+  labelColors: PropTypes.object,
   height: PropTypes.number,
   width: PropTypes.number,
   setControlValue: PropTypes.func,
@@ -100,6 +101,7 @@ class ChartRenderer extends React.Component {
         nextProps.height !== this.props.height ||
         nextProps.width !== this.props.width ||
         nextProps.triggerRender ||
+        nextProps.labelColors !== this.props.labelColors ||
         nextProps.formData.color_scheme !== this.props.formData.color_scheme ||
         nextProps.cacheBusterProp !== this.props.cacheBusterProp
       );

diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts
@@ -17,13 +17,32 @@
  * under the License.
  */
 import { Dispatch } from 'redux';
-import { makeApi } from '@superset-ui/core';
+import { makeApi, CategoricalColorNamespace } from '@superset-ui/core';
+import { isString } from 'lodash';
 import { ChartConfiguration, DashboardInfo } from '../reducers/types';
 
 export const DASHBOARD_INFO_UPDATED = 'DASHBOARD_INFO_UPDATED';
 
 // updates partially changed dashboard info
 export function dashboardInfoChanged(newInfo: { metadata: any }) {
+  const { metadata } = newInfo;
+
+  const categoricalNamespace = CategoricalColorNamespace.getNamespace(
+    metadata?.color_namespace,
+  );
+
+  categoricalNamespace.resetColors();
+
+  if (metadata?.label_colors) {
+    const labelColors = metadata.label_colors;
+    const colorMap = isString(labelColors)
+      ? JSON.parse(labelColors)
+      : labelColors;
+    Object.keys(colorMap).forEach(label => {
+      categoricalNamespace.setColor(label, colorMap[label]);
+    });
+  }
+
   return { type: DASHBOARD_INFO_UPDATED, newInfo };
 }
 export const SET_CHART_CONFIG_BEGIN = 'SET_CHART_CONFIG_BEGIN';

diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js
@@ -88,16 +88,16 @@ export const hydrateDashboard = (dashboardData, chartData) => (
   // Priming the color palette with user's label-color mapping provided in
   // the dashboard's JSON metadata
   if (metadata?.label_colors) {
-    const scheme = metadata.color_scheme;
     const namespace = metadata.color_namespace;
     const colorMap = isString(metadata.label_colors)
       ? JSON.parse(metadata.label_colors)
       : metadata.label_colors;
+    const categoricalNamespace = CategoricalColorNamespace.getNamespace(
+      namespace,
+    );
+
     Object.keys(colorMap).forEach(label => {
-      CategoricalColorNamespace.getScale(scheme, namespace).setColor(
-        label,
-        colorMap[label],
-      );
+      categoricalNamespace.setColor(label, colorMap[label]);
     });
   }
 

diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx
@@ -20,7 +20,7 @@
 import moment from 'moment';
 import React from 'react';
 import PropTypes from 'prop-types';
-import { styled, CategoricalColorNamespace, t } from '@superset-ui/core';
+import { styled, t } from '@superset-ui/core';
 import ButtonGroup from 'src/components/ButtonGroup';
 
 import {
@@ -348,19 +348,10 @@ class Header extends React.PureComponent {
       lastModifiedTime,
     } = this.props;
 
-    const scale = CategoricalColorNamespace.getScale(
-      colorScheme,
-      colorNamespace,
-    );
-
-    // use the colorScheme for default labels
-    let labelColors = colorScheme ? scale.getColorMap() : {};
-    // but allow metadata to overwrite if it exists
-    // eslint-disable-next-line camelcase
-    const metadataLabelColors = dashboardInfo.metadata?.label_colors;
-    if (metadataLabelColors) {
-      labelColors = { ...labelColors, ...metadataLabelColors };
-    }
+    const labelColors =
+      colorScheme && dashboardInfo?.metadata?.label_colors
+        ? dashboardInfo.metadata.label_colors
+        : {};
 
     // check refresh frequency is for current session or persist
     const refreshFrequency = shouldPersistRefreshFrequency

diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
@@ -29,7 +29,6 @@ import {
   t,
   SupersetClient,
   getCategoricalSchemeRegistry,
-  CategoricalColorNamespace,
 } from '@superset-ui/core';
 
 import Modal from 'src/components/Modal';
@@ -140,14 +139,15 @@ class PropertiesModal extends React.PureComponent {
     JsonEditor.preload();
   }
 
-  onColorSchemeChange(value, { updateMetadata = true } = {}) {
+  onColorSchemeChange(colorScheme, { updateMetadata = true } = {}) {
     // check that color_scheme is valid
     const colorChoices = getCategoricalSchemeRegistry().keys();
     const { json_metadata: jsonMetadata } = this.state.values;
     const jsonMetadataObj = jsonMetadata?.length
       ? JSON.parse(jsonMetadata)
       : {};
-    if (!colorChoices.includes(value)) {
+
+    if (!colorScheme || !colorChoices.includes(colorScheme)) {
       Modal.error({
         title: 'Error',
         content: t('A valid color scheme is required'),
@@ -157,24 +157,14 @@ class PropertiesModal extends React.PureComponent {
     }
 
     // update metadata to match selection
-    if (
-      updateMetadata &&
-      Object.keys(jsonMetadataObj).includes('color_scheme')
-    ) {
-      jsonMetadataObj.color_scheme = value;
-      jsonMetadataObj.label_colors = Object.keys(
-        jsonMetadataObj.label_colors ?? {},
-      ).reduce(
-        (prev, next) => ({
-          ...prev,
-          [next]: CategoricalColorNamespace.getScale(value)(next),
-        }),
-        {},
-      );
+    if (updateMetadata) {
+      jsonMetadataObj.color_scheme = colorScheme;
+      jsonMetadataObj.label_colors = jsonMetadataObj.label_colors || {};
+
       this.onMetadataChange(jsonStringify(jsonMetadataObj));
     }
 
-    this.updateFormState('colorScheme', value);
+    this.updateFormState('colorScheme', colorScheme);
   }
 
   onOwnersChange(value) {
@@ -261,21 +251,22 @@ class PropertiesModal extends React.PureComponent {
         roles: rolesValue,
       },
     } = this.state;
+
     const { onlyApply } = this.props;
     const owners = ownersValue?.map(o => o.value) ?? [];
     const roles = rolesValue?.map(o => o.value) ?? [];
-    let metadataColorScheme;
+    let currentColorScheme = colorScheme;
 
-    // update color scheme to match metadata
+    // color scheme in json metadata has precedence over selection
     if (jsonMetadata?.length) {
-      const { color_scheme: metadataColorScheme } = JSON.parse(jsonMetadata);
-      if (metadataColorScheme) {
-        this.onColorSchemeChange(metadataColorScheme, {
-          updateMetadata: false,
-        });
-      }
+      const metadata = JSON.parse(jsonMetadata);
+      currentColorScheme = metadata?.color_scheme || colorScheme;
     }
 
+    this.onColorSchemeChange(currentColorScheme, {
+      updateMetadata: false,
+    });
+
     const moreProps = {};
     const morePutProps = {};
     if (isFeatureEnabled(FeatureFlag.DASHBOARD_RBAC)) {
@@ -289,7 +280,7 @@ class PropertiesModal extends React.PureComponent {
         slug,
         jsonMetadata,
         ownerIds: owners,
-        colorScheme: metadataColorScheme || colorScheme,
+        colorScheme: currentColorScheme,
         ...moreProps,
       });
       this.props.onHide();
@@ -316,7 +307,7 @@ class PropertiesModal extends React.PureComponent {
           slug: result.slug,
           jsonMetadata: result.json_metadata,
           ownerIds: result.owners,
-          colorScheme: metadataColorScheme || colorScheme,
+          colorScheme: currentColorScheme,
           ...moreResultProps,
         });
         this.props.onHide();