From 7a7181a2449598b09298f3a113849caeb3309186 Mon Sep 17 00:00:00 2001 From: Stephen Liu <750188453@qq.com> Date: Mon, 17 Oct 2022 17:01:20 +0800 Subject: [PATCH] feat(color): color consistency enhancements (#21507) --- .../src/color/CategoricalColorScale.ts | 24 ++- .../src/color/SharedLabelColorSingleton.ts | 148 +++++++----------- .../superset-ui-core/src/color/index.ts | 1 + .../test/color/CategoricalColorScale.test.ts | 2 +- .../color/SharedLabelColorSingleton.test.ts | 121 +++++++++++--- .../src/dashboard/actions/dashboardState.js | 40 ----- .../dashboard/actions/dashboardState.test.js | 2 +- .../components/Header/Header.test.tsx | 3 +- .../src/dashboard/components/Header/index.jsx | 15 +- .../src/dashboard/components/Header/types.ts | 3 +- .../PropertiesModal/PropertiesModal.test.tsx | 2 +- .../components/PropertiesModal/index.tsx | 32 ++-- .../components/gridComponents/Chart.jsx | 9 ++ .../components/gridComponents/ChartHolder.tsx | 12 -- .../containers/DashboardComponent.jsx | 2 - .../dashboard/containers/DashboardHeader.jsx | 6 +- .../dashboard/containers/DashboardPage.tsx | 14 +- .../src/dashboard/reducers/dashboardState.js | 9 -- .../dashboard/reducers/dashboardState.test.js | 9 +- superset-frontend/src/explore/ExplorePage.tsx | 10 +- superset/config.py | 2 +- 21 files changed, 229 insertions(+), 237 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 e254730ac317b..5f29ad4775996 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -51,7 +51,7 @@ class CategoricalColorScale extends ExtensibleFunction { * @param {*} parentForcedColors optional parameter that comes from parent * (usually CategoricalColorNamespace) and supersede this.forcedColors */ - constructor(colors: string[], parentForcedColors?: ColorsLookup) { + constructor(colors: string[], parentForcedColors: ColorsLookup = {}) { super((value: string, sliceId?: number) => this.getColor(value, sliceId)); this.originColors = colors; @@ -67,17 +67,11 @@ class CategoricalColorScale extends ExtensibleFunction { const cleanedValue = stringifyAndTrim(value); const sharedLabelColor = getSharedLabelColor(); - const parentColor = this.parentForcedColors?.[cleanedValue]; - if (parentColor) { - sharedLabelColor.addSlice(cleanedValue, parentColor, sliceId); - return parentColor; - } - - const forcedColor = this.forcedColors[cleanedValue]; - if (forcedColor) { - sharedLabelColor.addSlice(cleanedValue, forcedColor, sliceId); - return forcedColor; - } + // priority: parentForcedColors > forcedColors > labelColors + let color = + this.parentForcedColors?.[cleanedValue] || + this.forcedColors?.[cleanedValue] || + sharedLabelColor.getColorMap().get(cleanedValue); if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { const multiple = Math.floor( @@ -89,8 +83,10 @@ class CategoricalColorScale extends ExtensibleFunction { this.range(this.originColors.concat(newRange)); } } - - const color = this.scale(cleanedValue); + const newColor = this.scale(cleanedValue); + if (!color) { + color = newColor; + } sharedLabelColor.addSlice(cleanedValue, color, sliceId); return color; diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts index 10a14df075910..bc417e6036a51 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -18,113 +18,81 @@ */ import { CategoricalColorNamespace } from '.'; -import { FeatureFlag, isFeatureEnabled, makeSingleton } from '../utils'; -import { getAnalogousColors } from './utils'; +import { makeSingleton } from '../utils'; +export enum SharedLabelColorSource { + dashboard, + explore, +} export class SharedLabelColor { - sliceLabelColorMap: Record>; + sliceLabelMap: Map; - constructor() { - // { sliceId1: { label1: color1 }, sliceId2: { label2: color2 } } - this.sliceLabelColorMap = {}; - } + colorMap: Map; - getColorMap( - colorNamespace?: string, - colorScheme?: string, - updateColorScheme?: boolean, - ) { - if (colorScheme) { - const categoricalNamespace = - CategoricalColorNamespace.getNamespace(colorNamespace); - const sharedLabels = this.getSharedLabels(); - let generatedColors: string[] = []; - let sharedLabelMap; + source: SharedLabelColorSource; - if (sharedLabels.length) { - const colorScale = categoricalNamespace.getScale(colorScheme); - const colors = colorScale.range(); - if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { - const multiple = Math.ceil(sharedLabels.length / colors.length); - generatedColors = getAnalogousColors(colors, multiple); - sharedLabelMap = sharedLabels.reduce( - (res, label, index) => ({ - ...res, - [label.toString()]: generatedColors[index], - }), - {}, - ); - } else { - // reverse colors to reduce color conflicts - colorScale.range(colors.reverse()); - sharedLabelMap = sharedLabels.reduce( - (res, label) => ({ - ...res, - [label.toString()]: colorScale(label), - }), - {}, - ); - } - } + constructor() { + // { sliceId1: [label1, label2, ...], sliceId2: [label1, label2, ...] } + this.sliceLabelMap = new Map(); + this.colorMap = new Map(); + this.source = SharedLabelColorSource.dashboard; + } - const labelMap = Object.keys(this.sliceLabelColorMap).reduce( - (res, sliceId) => { - // get new color scale instance - const colorScale = categoricalNamespace.getScale(colorScheme); - return { - ...res, - ...Object.keys(this.sliceLabelColorMap[sliceId]).reduce( - (res, label) => ({ - ...res, - [label]: updateColorScheme - ? colorScale(label) - : this.sliceLabelColorMap[sliceId][label], - }), - {}, - ), - }; - }, - {}, - ); + updateColorMap(colorNamespace?: string, colorScheme?: string) { + const categoricalNamespace = + CategoricalColorNamespace.getNamespace(colorNamespace); + const newColorMap = new Map(); + this.colorMap.clear(); + this.sliceLabelMap.forEach(labels => { + const colorScale = categoricalNamespace.getScale(colorScheme); + labels.forEach(label => { + const newColor = colorScale(label); + newColorMap.set(label, newColor); + }); + }); + this.colorMap = newColorMap; + } - return { - ...labelMap, - ...sharedLabelMap, - }; - } - return undefined; + getColorMap() { + return this.colorMap; } addSlice(label: string, color: string, sliceId?: number) { - if (!sliceId) return; - this.sliceLabelColorMap[sliceId] = { - ...this.sliceLabelColorMap[sliceId], - [label]: color, - }; + if ( + this.source !== SharedLabelColorSource.dashboard || + sliceId === undefined + ) + return; + const labels = this.sliceLabelMap.get(sliceId) || []; + if (!labels.includes(label)) { + labels.push(label); + this.sliceLabelMap.set(sliceId, labels); + } + this.colorMap.set(label, color); } removeSlice(sliceId: number) { - delete this.sliceLabelColorMap[sliceId]; + if (this.source !== SharedLabelColorSource.dashboard) return; + this.sliceLabelMap.delete(sliceId); + const newColorMap = new Map(); + this.sliceLabelMap.forEach(labels => { + labels.forEach(label => { + newColorMap.set(label, this.colorMap.get(label)); + }); + }); + this.colorMap = newColorMap; } - clear() { - this.sliceLabelColorMap = {}; + reset() { + const copyColorMap = new Map(this.colorMap); + copyColorMap.forEach((_, label) => { + this.colorMap.set(label, ''); + }); } - getSharedLabels() { - const tempLabels = new Set(); - const result = new Set(); - Object.keys(this.sliceLabelColorMap).forEach(sliceId => { - const colorMap = this.sliceLabelColorMap[sliceId]; - Object.keys(colorMap).forEach(label => { - if (tempLabels.has(label) && !result.has(label)) { - result.add(label); - } else { - tempLabels.add(label); - } - }); - }); - return [...result]; + clear() { + this.sliceLabelMap.clear(); + this.colorMap.clear(); } } diff --git a/superset-frontend/packages/superset-ui-core/src/color/index.ts b/superset-frontend/packages/superset-ui-core/src/color/index.ts index e1cde3ba3e2d5..3bbdb5d0dc578 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/index.ts @@ -35,6 +35,7 @@ export * from './utils'; export { default as getSharedLabelColor, SharedLabelColor, + SharedLabelColorSource, } from './SharedLabelColorSingleton'; export const BRAND_COLOR = '#00A699'; 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 91a8f4a3185a7..9e83aaba9a871 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 @@ -140,7 +140,7 @@ describe('CategoricalColorScale', () => { expect(scale2.getColorMap()).toEqual({ cow: 'black', pig: 'pink', - horse: 'blue', + horse: 'green', }); }); }); diff --git a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts index 86d3ba9c409e1..88610874dbd74 100644 --- a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts @@ -23,6 +23,7 @@ import { getCategoricalSchemeRegistry, getSharedLabelColor, SharedLabelColor, + SharedLabelColorSource, } from '@superset-ui/core'; import { getAnalogousColors } from '../../src/color/utils'; @@ -52,6 +53,7 @@ describe('SharedLabelColor', () => { }); beforeEach(() => { + getSharedLabelColor().source = SharedLabelColorSource.dashboard; getSharedLabelColor().clear(); }); @@ -60,18 +62,48 @@ describe('SharedLabelColor', () => { }); describe('.addSlice(value, color, sliceId)', () => { - it('should add to valueSliceMap when first adding label', () => { + it('should add to sliceLabelColorMap when first adding label', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - expect(sharedLabelColor.sliceLabelColorMap).toHaveProperty('1', { - a: 'red', - }); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(true); + const labels = sharedLabelColor.sliceLabelMap.get(1); + expect(labels?.includes('a')).toEqual(true); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red' }); + }); + + it('should add to sliceLabelColorMap when slice exist', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 1); + const labels = sharedLabelColor.sliceLabelMap.get(1); + expect(labels?.includes('b')).toEqual(true); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red', b: 'blue' }); + }); + + it('should use last color if adding label repeatedly', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('b', 'blue', 1); + sharedLabelColor.addSlice('b', 'green', 1); + const labels = sharedLabelColor.sliceLabelMap.get(1); + expect(labels?.includes('b')).toEqual(true); + expect(labels?.length).toEqual(1); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ b: 'green' }); + }); + + it('should do nothing when source is not dashboard', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.source = SharedLabelColorSource.explore; + sharedLabelColor.addSlice('a', 'red'); + expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); }); it('should do nothing when sliceId is undefined', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red'); - expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); }); }); @@ -80,55 +112,92 @@ describe('SharedLabelColor', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.removeSlice(1); - expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(false); }); - }); - describe('.getColorMap(namespace, scheme, updateColorScheme)', () => { - it('should be undefined when scheme is undefined', () => { + it('should update colorMap', () => { const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 2); + sharedLabelColor.removeSlice(1); const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).toBeUndefined(); + expect(Object.fromEntries(colorMap)).toEqual({ b: 'blue' }); }); - it('should update color value if passing updateColorScheme', () => { + it('should do nothing when source is not dashboard', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.addSlice('b', 'blue', 2); - const colorMap = sharedLabelColor.getColorMap('', 'testColors2', true); - expect(colorMap).toEqual({ a: 'yellow', b: 'yellow' }); + sharedLabelColor.source = SharedLabelColorSource.explore; + sharedLabelColor.removeSlice(1); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(true); }); + }); - it('should get origin color value if not pass updateColorScheme', () => { + describe('.updateColorMap(namespace, scheme)', () => { + it('should update color map', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.addSlice('b', 'blue', 2); - const colorMap = sharedLabelColor.getColorMap('', 'testColors'); - expect(colorMap).toEqual({ a: 'red', b: 'blue' }); + sharedLabelColor.addSlice('b', 'pink', 1); + sharedLabelColor.addSlice('b', 'green', 2); + sharedLabelColor.addSlice('c', 'blue', 2); + sharedLabelColor.updateColorMap('', 'testColors2'); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ + a: 'yellow', + b: 'yellow', + c: 'green', + }); }); - it('should use recycle colors if shared label exit', () => { + it('should use recycle colors', () => { window.featureFlags = { [FeatureFlag.USE_ANALAGOUS_COLORS]: false, }; const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.addSlice('a', 'blue', 2); - const colorMap = sharedLabelColor.getColorMap('', 'testColors'); - expect(colorMap).not.toEqual({}); + sharedLabelColor.addSlice('b', 'blue', 2); + sharedLabelColor.addSlice('c', 'green', 3); + sharedLabelColor.addSlice('d', 'red', 4); + sharedLabelColor.updateColorMap('', 'testColors'); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).not.toEqual({}); expect(getAnalogousColors).not.toBeCalled(); }); - it('should use analagous colors if shared label exit', () => { + it('should use analagous colors', () => { window.featureFlags = { [FeatureFlag.USE_ANALAGOUS_COLORS]: true, }; const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.addSlice('a', 'blue', 2); - const colorMap = sharedLabelColor.getColorMap('', 'testColors'); - expect(colorMap).not.toEqual({}); + sharedLabelColor.addSlice('b', 'blue', 1); + sharedLabelColor.addSlice('c', 'green', 1); + sharedLabelColor.addSlice('d', 'red', 1); + sharedLabelColor.updateColorMap('', 'testColors'); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).not.toEqual({}); expect(getAnalogousColors).toBeCalled(); }); }); + + describe('.getColorMap()', () => { + it('should get color map', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 2); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red', b: 'blue' }); + }); + }); + + describe('.reset()', () => { + it('should reset color map', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 2); + sharedLabelColor.reset(); + const colorMap = sharedLabelColor.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: '', b: '' }); + }); + }); }); diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index c649305dd3084..701eab78454d4 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -72,11 +72,6 @@ export function removeSlice(sliceId) { return { type: REMOVE_SLICE, sliceId }; } -export const RESET_SLICE = 'RESET_SLICE'; -export function resetSlice() { - return { type: RESET_SLICE }; -} - const FAVESTAR_BASE_URL = '/superset/favstar/Dashboard'; export const TOGGLE_FAVE_STAR = 'TOGGLE_FAVE_STAR'; export function toggleFaveStar(isStarred) { @@ -506,28 +501,6 @@ export function addSliceToDashboard(id, component) { }; } -export function postAddSliceFromDashboard() { - return (dispatch, getState) => { - const { - dashboardInfo: { metadata }, - dashboardState, - } = getState(); - - if (dashboardState?.updateSlice && dashboardState?.editMode) { - metadata.shared_label_colors = getSharedLabelColor().getColorMap( - metadata?.color_namespace, - metadata?.color_scheme, - ); - dispatch( - dashboardInfoChanged({ - metadata, - }), - ); - dispatch(resetSlice()); - } - }; -} - export function removeSliceFromDashboard(id) { return (dispatch, getState) => { const sliceEntity = getState().sliceEntities.slices[id]; @@ -537,20 +510,7 @@ export function removeSliceFromDashboard(id) { dispatch(removeSlice(id)); dispatch(removeChart(id)); - - const { - dashboardInfo: { metadata }, - } = getState(); getSharedLabelColor().removeSlice(id); - metadata.shared_label_colors = getSharedLabelColor().getColorMap( - metadata?.color_namespace, - metadata?.color_scheme, - ); - dispatch( - dashboardInfoChanged({ - metadata, - }), - ); }; } diff --git a/superset-frontend/src/dashboard/actions/dashboardState.test.js b/superset-frontend/src/dashboard/actions/dashboardState.test.js index f5fa60c08d56d..c985496f892d8 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.test.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.test.js @@ -120,6 +120,6 @@ describe('dashboardState actions', () => { const removeFilter = dispatch.getCall(0).args[0]; removeFilter(dispatch, getState); - expect(dispatch.getCall(4).args[0].type).toBe(REMOVE_FILTER); + expect(dispatch.getCall(3).args[0].type).toBe(REMOVE_FILTER); }); }); diff --git a/superset-frontend/src/dashboard/components/Header/Header.test.tsx b/superset-frontend/src/dashboard/components/Header/Header.test.tsx index 010e88e576da4..2eb73091bcd82 100644 --- a/superset-frontend/src/dashboard/components/Header/Header.test.tsx +++ b/superset-frontend/src/dashboard/components/Header/Header.test.tsx @@ -77,7 +77,8 @@ const createProps = () => ({ setEditMode: jest.fn(), showBuilderPane: jest.fn(), updateCss: jest.fn(), - setColorSchemeAndUnsavedChanges: jest.fn(), + setColorScheme: jest.fn(), + setUnsavedChanges: jest.fn(), logEvent: jest.fn(), setRefreshFrequency: jest.fn(), hasUnsavedChanges: false, diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 354ee81bbcfe2..178f6da6eeb43 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -75,7 +75,8 @@ const propTypes = { customCss: PropTypes.string.isRequired, colorNamespace: PropTypes.string, colorScheme: PropTypes.string, - setColorSchemeAndUnsavedChanges: PropTypes.func.isRequired, + setColorScheme: PropTypes.func.isRequired, + setUnsavedChanges: PropTypes.func.isRequired, isStarred: PropTypes.bool.isRequired, isPublished: PropTypes.bool.isRequired, isLoading: PropTypes.bool.isRequired, @@ -371,9 +372,8 @@ class Header extends React.PureComponent { dashboardInfo?.metadata?.color_scheme || colorScheme; const currentColorNamespace = dashboardInfo?.metadata?.color_namespace || colorNamespace; - const currentSharedLabelColors = getSharedLabelColor().getColorMap( - currentColorNamespace, - currentColorScheme, + const currentSharedLabelColors = Object.fromEntries( + getSharedLabelColor().getColorMap(), ); const data = { @@ -439,7 +439,8 @@ class Header extends React.PureComponent { customCss, colorNamespace, dataMask, - setColorSchemeAndUnsavedChanges, + setColorScheme, + setUnsavedChanges, colorScheme, onUndo, onRedo, @@ -480,6 +481,8 @@ class Header extends React.PureComponent { const handleOnPropertiesChange = updates => { const { dashboardInfoChanged, dashboardTitleChanged } = this.props; + + setColorScheme(updates.colorScheme); dashboardInfoChanged({ slug: updates.slug, metadata: JSON.parse(updates.jsonMetadata || '{}'), @@ -488,7 +491,7 @@ class Header extends React.PureComponent { owners: updates.owners, roles: updates.roles, }); - setColorSchemeAndUnsavedChanges(updates.colorScheme); + setUnsavedChanges(true); dashboardTitleChanged(updates.title); }; diff --git a/superset-frontend/src/dashboard/components/Header/types.ts b/superset-frontend/src/dashboard/components/Header/types.ts index f03e01874005f..89ea7569888d1 100644 --- a/superset-frontend/src/dashboard/components/Header/types.ts +++ b/superset-frontend/src/dashboard/components/Header/types.ts @@ -68,7 +68,8 @@ export interface HeaderProps { user: Object | undefined; dashboardInfo: DashboardInfo; dashboardTitle: string; - setColorSchemeAndUnsavedChanges: () => void; + setColorScheme: () => void; + setUnsavedChanges: () => void; isStarred: boolean; isPublished: boolean; onChange: () => void; diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx index 8b09a57d54370..645b70cc55fb0 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx @@ -309,7 +309,7 @@ test('submitting with onlyApply:false', async () => { certificationDetails: 'Sample certification', certifiedBy: 'John Doe', colorScheme: 'supersetColors', - colorNamespace: '', + colorNamespace: undefined, id: 26, jsonMetadata: expect.anything(), owners: [], diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 9fb63d3430050..d98daadff96cd 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -24,6 +24,7 @@ import Button from 'src/components/Button'; import { AntdForm, AsyncSelect, Col, Row } from 'src/components'; import rison from 'rison'; import { + CategoricalColorNamespace, ensureIsArray, getCategoricalSchemeRegistry, getSharedLabelColor, @@ -57,7 +58,6 @@ type PropertiesModalProps = { show?: boolean; onHide?: () => void; colorScheme?: string; - setColorSchemeAndUnsavedChanges?: () => void; onSubmit?: (params: Record) => void; addSuccessToast: (message: string) => void; addDangerToast: (message: string) => void; @@ -181,9 +181,7 @@ const PropertiesModal = ({ } const metaDataCopy = { ...metadata }; - if (metaDataCopy?.shared_label_colors) { - delete metaDataCopy.shared_label_colors; - } + delete metaDataCopy.shared_label_colors; delete metaDataCopy.color_scheme_domain; @@ -268,7 +266,7 @@ const PropertiesModal = ({ }; const onColorSchemeChange = ( - colorScheme?: string, + colorScheme = '', { updateMetadata = true } = {}, ) => { // check that color_scheme is valid @@ -319,7 +317,7 @@ const PropertiesModal = ({ // color scheme in json metadata has precedence over selection currentColorScheme = metadata?.color_scheme || colorScheme; - colorNamespace = metadata?.color_namespace || ''; + colorNamespace = metadata?.color_namespace; // filter shared_label_color from user input if (metadata?.shared_label_colors) { @@ -329,16 +327,20 @@ const PropertiesModal = ({ delete metadata.color_scheme_domain; } - metadata.shared_label_colors = getSharedLabelColor().getColorMap( - colorNamespace, - currentColorScheme, - true, - ); - - if (metadata?.color_scheme) { + const sharedLabelColor = getSharedLabelColor(); + const categoricalNamespace = + CategoricalColorNamespace.getNamespace(colorNamespace); + categoricalNamespace.resetColors(); + if (currentColorScheme) { + sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme); + metadata.shared_label_colors = Object.fromEntries( + sharedLabelColor.getColorMap(), + ); metadata.color_scheme_domain = categoricalSchemeRegistry.get(colorScheme)?.colors || []; } else { + sharedLabelColor.reset(); + metadata.shared_label_colors = {}; metadata.color_scheme_domain = []; } @@ -367,9 +369,9 @@ const PropertiesModal = ({ ...moreOnSubmitProps, }; if (onlyApply) { - addSuccessToast(t('Dashboard properties updated')); onSubmit(onSubmitProps); onHide(); + addSuccessToast(t('Dashboard properties updated')); } else { SupersetClient.put({ endpoint: `/api/v1/dashboard/${dashboardId}`, @@ -385,9 +387,9 @@ const PropertiesModal = ({ ...morePutProps, }), }).then(() => { - addSuccessToast(t('The dashboard has been saved')); onSubmit(onSubmitProps); onHide(); + addSuccessToast(t('The dashboard has been saved')); }, handleErrorResponse); } }; diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 9e3e3e94b61d4..0199ce50b674b 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -200,6 +200,15 @@ class Chart extends React.Component { return true; } } + } else if ( + // chart should re-render if color scheme or label color was changed + nextProps.formData?.color_scheme !== this.props.formData?.color_scheme || + !areObjectsEqual( + nextProps.formData?.label_colors, + this.props.formData?.label_colors, + ) + ) { + return true; } // `cacheBusterProp` is jected by react-hot-loader diff --git a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx index 103fbf273efa2..30626a6ba5803 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx @@ -66,8 +66,6 @@ interface ChartHolderProps { updateComponents: Function; handleComponentDrop: (...args: unknown[]) => unknown; setFullSizeChartId: (chartId: number | null) => void; - postAddSliceFromDashboard?: () => void; - isInView: boolean; } @@ -92,7 +90,6 @@ const ChartHolder: React.FC = ({ updateComponents, handleComponentDrop, setFullSizeChartId, - postAddSliceFromDashboard, isInView, }) => { const { chartId } = component.meta; @@ -236,14 +233,6 @@ const ChartHolder: React.FC = ({ })); }, []); - const handlePostTransformProps = useCallback( - (props: unknown) => { - postAddSliceFromDashboard?.(); - return props; - }, - [postAddSliceFromDashboard], - ); - return ( = ({ isFullSize={isFullSize} setControlValue={handleExtraControl} extraControls={extraControls} - postTransformProps={handlePostTransformProps} isInView={isInView} /> {editMode && ( diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx index 23298d8bf96a4..08b7ed9f82d90 100644 --- a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx +++ b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx @@ -37,7 +37,6 @@ import { setDirectPathToChild, setActiveTabs, setFullSizeChartId, - postAddSliceFromDashboard, } from 'src/dashboard/actions/dashboardState'; const propTypes = { @@ -112,7 +111,6 @@ function mapDispatchToProps(dispatch) { setFullSizeChartId, setActiveTabs, logEvent, - postAddSliceFromDashboard, }, dispatch, ); diff --git a/superset-frontend/src/dashboard/containers/DashboardHeader.jsx b/superset-frontend/src/dashboard/containers/DashboardHeader.jsx index 121215fe8bfea..178568f064eaa 100644 --- a/superset-frontend/src/dashboard/containers/DashboardHeader.jsx +++ b/superset-frontend/src/dashboard/containers/DashboardHeader.jsx @@ -31,7 +31,8 @@ import { fetchFaveStar, saveFaveStar, savePublished, - setColorSchemeAndUnsavedChanges, + setColorScheme, + setUnsavedChanges, fetchCharts, updateCss, onChange, @@ -112,7 +113,8 @@ function mapDispatchToProps(dispatch) { onRedo: redoLayoutAction, setEditMode, showBuilderPane, - setColorSchemeAndUnsavedChanges, + setColorScheme, + setUnsavedChanges, fetchFaveStar, saveFaveStar, savePublished, diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index 598ed3e6e3877..a835523ae7684 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -23,6 +23,7 @@ import { FeatureFlag, getSharedLabelColor, isFeatureEnabled, + SharedLabelColorSource, t, useTheme, } from '@superset-ui/core'; @@ -336,17 +337,18 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { return () => {}; }, [css]); - useEffect( - () => () => { + useEffect(() => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.source = SharedLabelColorSource.dashboard; + return () => { // clean up label color const categoricalNamespace = CategoricalColorNamespace.getNamespace( metadata?.color_namespace, ); categoricalNamespace.resetColors(); - getSharedLabelColor().clear(); - }, - [metadata?.color_namespace], - ); + sharedLabelColor.clear(); + }; + }, [metadata?.color_namespace]); useEffect(() => { if (datasetsApiError) { diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.js b/superset-frontend/src/dashboard/reducers/dashboardState.js index 277865030cb79..14b35caef04a7 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardState.js +++ b/superset-frontend/src/dashboard/reducers/dashboardState.js @@ -39,7 +39,6 @@ import { UNSET_FOCUSED_FILTER_FIELD, SET_ACTIVE_TABS, SET_FULL_SIZE_CHART_ID, - RESET_SLICE, ON_FILTERS_REFRESH, ON_FILTERS_REFRESH_SUCCESS, SET_DATASETS_STATUS, @@ -60,7 +59,6 @@ export default function dashboardStateReducer(state = {}, action) { return { ...state, sliceIds: Array.from(updatedSliceIds), - updateSlice: true, }; }, [REMOVE_SLICE]() { @@ -73,12 +71,6 @@ export default function dashboardStateReducer(state = {}, action) { sliceIds: Array.from(updatedSliceIds), }; }, - [RESET_SLICE]() { - return { - ...state, - updateSlice: false, - }; - }, [TOGGLE_FAVE_STAR]() { return { ...state, isStarred: action.isStarred }; }, @@ -125,7 +117,6 @@ export default function dashboardStateReducer(state = {}, action) { maxUndoHistoryExceeded: false, editMode: false, updatedColorScheme: false, - updateSlice: false, // server-side returns last_modified_time for latest change lastModifiedTime: action.lastModifiedTime, }; diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.test.js b/superset-frontend/src/dashboard/reducers/dashboardState.test.js index de3ecf72ff3ec..39798ecf139e5 100644 --- a/superset-frontend/src/dashboard/reducers/dashboardState.test.js +++ b/superset-frontend/src/dashboard/reducers/dashboardState.test.js @@ -28,7 +28,6 @@ import { TOGGLE_EXPAND_SLICE, TOGGLE_FAVE_STAR, UNSET_FOCUSED_FILTER_FIELD, - RESET_SLICE, } from 'src/dashboard/actions/dashboardState'; import dashboardStateReducer from 'src/dashboard/reducers/dashboardState'; @@ -44,7 +43,7 @@ describe('dashboardState reducer', () => { { sliceIds: [1] }, { type: ADD_SLICE, slice: { slice_id: 2 } }, ), - ).toEqual({ sliceIds: [1, 2], updateSlice: true }); + ).toEqual({ sliceIds: [1, 2] }); }); it('should remove a slice', () => { @@ -56,12 +55,6 @@ describe('dashboardState reducer', () => { ).toEqual({ sliceIds: [1], filters: {} }); }); - it('should reset updateSlice', () => { - expect( - dashboardStateReducer({ updateSlice: true }, { type: RESET_SLICE }), - ).toEqual({ updateSlice: false }); - }); - it('should toggle fav star', () => { expect( dashboardStateReducer( diff --git a/superset-frontend/src/explore/ExplorePage.tsx b/superset-frontend/src/explore/ExplorePage.tsx index c8d4138ce4c1c..ce36cfca252c9 100644 --- a/superset-frontend/src/explore/ExplorePage.tsx +++ b/superset-frontend/src/explore/ExplorePage.tsx @@ -19,7 +19,14 @@ import React, { useEffect, useRef, useState } from 'react'; import { useDispatch } from 'react-redux'; import { useLocation } from 'react-router-dom'; -import { isDefined, JsonObject, makeApi, t } from '@superset-ui/core'; +import { + getSharedLabelColor, + isDefined, + JsonObject, + makeApi, + SharedLabelColorSource, + t, +} from '@superset-ui/core'; import Loading from 'src/components/Loading'; import { addDangerToast } from 'src/components/MessageToasts/actions'; import { getUrlParam } from 'src/utils/urlUtils'; @@ -139,6 +146,7 @@ export default function ExplorePage() { isExploreInitialized.current = true; }); } + getSharedLabelColor().source = SharedLabelColorSource.explore; }, [dispatch, location]); if (!isLoaded) { diff --git a/superset/config.py b/superset/config.py index aec2dba890c0b..64dc3baa125b5 100644 --- a/superset/config.py +++ b/superset/config.py @@ -454,7 +454,7 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]: "UX_BETA": False, "GENERIC_CHART_AXES": False, "ALLOW_ADHOC_SUBQUERY": False, - "USE_ANALAGOUS_COLORS": True, + "USE_ANALAGOUS_COLORS": False, "DASHBOARD_EDIT_CHART_IN_NEW_TAB": False, # Apply RLS rules to SQL Lab queries. This requires parsing and manipulating the # query, and might break queries and/or allow users to bypass RLS. Use with care!