From e741ec90b810a880b4841fbbe87fe4d832db81d5 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Sun, 18 Sep 2022 23:55:31 +0800 Subject: [PATCH 01/13] feat(color): color consistency enhancements --- .../src/color/CategoricalColorScale.ts | 38 +++--- .../src/color/SharedLabelColorSingleton.ts | 109 +++++------------- .../color/SharedLabelColorSingleton.test.ts | 53 +++++---- .../src/dashboard/actions/dashboardState.js | 17 +-- .../src/dashboard/components/Header/index.jsx | 5 +- .../components/PropertiesModal/index.tsx | 12 +- .../components/gridComponents/Chart.jsx | 5 + 7 files changed, 86 insertions(+), 153 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..e14011e3f026c 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -67,30 +67,24 @@ 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; - } - - if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { - const multiple = Math.floor( - this.domain().length / this.originColors.length, - ); - if (multiple > this.multiple) { - this.multiple = multiple; - const newRange = getAnalogousColors(this.originColors, multiple); - this.range(this.originColors.concat(newRange)); + // priority: parentForcedColors > forcedColors + let color = + this.parentForcedColors?.[cleanedValue] || + this.forcedColors?.[cleanedValue]; + + if (!color) { + if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { + const multiple = Math.floor( + this.domain().length / this.originColors.length, + ); + if (multiple > this.multiple) { + this.multiple = multiple; + const newRange = getAnalogousColors(this.originColors, multiple); + this.range(this.originColors.concat(newRange)); + } } + color = this.scale(cleanedValue); } - - const color = this.scale(cleanedValue); 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..4ddd5b9150ddd 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -18,8 +18,7 @@ */ import { CategoricalColorNamespace } from '.'; -import { FeatureFlag, isFeatureEnabled, makeSingleton } from '../utils'; -import { getAnalogousColors } from './utils'; +import { makeSingleton } from '../utils'; export class SharedLabelColor { sliceLabelColorMap: Record>; @@ -29,70 +28,36 @@ export class SharedLabelColor { this.sliceLabelColorMap = {}; } - getColorMap( - colorNamespace?: string, - colorScheme?: string, - updateColorScheme?: boolean, - ) { - if (colorScheme) { - const categoricalNamespace = - CategoricalColorNamespace.getNamespace(colorNamespace); - const sharedLabels = this.getSharedLabels(); - let generatedColors: string[] = []; - let sharedLabelMap; - - 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), - }), - {}, - ); - } - } + updateColorMap(colorNamespace?: string, colorScheme?: string) { + const categoricalNamespace = + CategoricalColorNamespace.getNamespace(colorNamespace); + const colorScale = categoricalNamespace.getScale(colorScheme); + const newSliceLabelColorMap = {}; + Object.keys(this.sliceLabelColorMap).forEach(sliceId => { + Object.keys(this.sliceLabelColorMap[sliceId]).forEach(label => { + newSliceLabelColorMap[sliceId] = { + ...newSliceLabelColorMap[sliceId], + [label]: colorScale(label), + }; + }); + }); + this.sliceLabelColorMap = newSliceLabelColorMap; + } - const labelMap = Object.keys(this.sliceLabelColorMap).reduce( - (res, sliceId) => { - // get new color scale instance - const colorScale = categoricalNamespace.getScale(colorScheme); - return { + getColorMap() { + return Object.keys(this.sliceLabelColorMap).reduce( + (res, sliceId) => ({ + ...res, + ...Object.keys(this.sliceLabelColorMap[sliceId]).reduce( + (res, label) => ({ ...res, - ...Object.keys(this.sliceLabelColorMap[sliceId]).reduce( - (res, label) => ({ - ...res, - [label]: updateColorScheme - ? colorScale(label) - : this.sliceLabelColorMap[sliceId][label], - }), - {}, - ), - }; - }, - {}, - ); - - return { - ...labelMap, - ...sharedLabelMap, - }; - } - return undefined; + [label]: this.sliceLabelColorMap[sliceId][label], + }), + {}, + ), + }), + {} as Record, + ); } addSlice(label: string, color: string, sliceId?: number) { @@ -110,22 +75,6 @@ export class SharedLabelColor { clear() { this.sliceLabelColorMap = {}; } - - 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]; - } } const getInstance = makeSingleton(SharedLabelColor); 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..8aa97349b707f 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 @@ -84,51 +84,54 @@ describe('SharedLabelColor', () => { }); }); - describe('.getColorMap(namespace, scheme, updateColorScheme)', () => { - it('should be undefined when scheme is undefined', () => { - const sharedLabelColor = getSharedLabelColor(); - const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).toBeUndefined(); - }); - - it('should update color value if passing updateColorScheme', () => { - 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' }); - }); - - 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', 'green', 2); + sharedLabelColor.updateColorMap('', 'testColors2'); + const colorMap = sharedLabelColor.getColorMap(); + expect(colorMap).toEqual({ a: 'yellow', b: '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'); + sharedLabelColor.addSlice('b', 'blue', 2); + sharedLabelColor.addSlice('c', 'green', 3); + sharedLabelColor.addSlice('d', 'red', 4); + sharedLabelColor.updateColorMap('', 'testColors'); + const colorMap = sharedLabelColor.getColorMap(); expect(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'); + sharedLabelColor.addSlice('b', 'blue', 2); + sharedLabelColor.addSlice('c', 'green', 3); + sharedLabelColor.addSlice('d', 'red', 4); + sharedLabelColor.updateColorMap('', 'testColors'); + const colorMap = sharedLabelColor.getColorMap(); expect(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(colorMap).toEqual({ a: 'red', b: 'blue' }); + }); + }); }); diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index c649305dd3084..b1fd92ea9fa0f 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -512,12 +512,14 @@ export function postAddSliceFromDashboard() { dashboardInfo: { metadata }, dashboardState, } = getState(); + const sharedLabelColor = getSharedLabelColor(); if (dashboardState?.updateSlice && dashboardState?.editMode) { - metadata.shared_label_colors = getSharedLabelColor().getColorMap( + sharedLabelColor.updateColorMap( metadata?.color_namespace, metadata?.color_scheme, ); + metadata.shared_label_colors = sharedLabelColor.getColorMap(); dispatch( dashboardInfoChanged({ metadata, @@ -537,20 +539,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/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 354ee81bbcfe2..c41126d9f5e7f 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -371,10 +371,7 @@ 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 = getSharedLabelColor().getColorMap(); const data = { certified_by: dashboardInfo.certified_by, diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 9fb63d3430050..7d46850f93dfa 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -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; @@ -329,11 +327,9 @@ const PropertiesModal = ({ delete metadata.color_scheme_domain; } - metadata.shared_label_colors = getSharedLabelColor().getColorMap( - colorNamespace, - currentColorScheme, - true, - ); + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme); + metadata.shared_label_colors = sharedLabelColor.getColorMap(); if (metadata?.color_scheme) { metadata.color_scheme_domain = diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 314dd6d1cb0ea..915cd381178e8 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -199,6 +199,11 @@ class Chart extends React.Component { return true; } } + } else if ( + // chart should re-render if color scheme was changed + nextProps.formData?.color_scheme !== this.props.formData?.color_scheme + ) { + return true; } // `cacheBusterProp` is jected by react-hot-loader From f5366825ac4e34e0858f9cf747c86e80238b33fd Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Wed, 21 Sep 2022 22:20:21 +0800 Subject: [PATCH 02/13] fix when label color was changed --- .../src/dashboard/actions/dashboardState.test.js | 2 +- .../src/dashboard/components/gridComponents/Chart.jsx | 8 ++++++-- superset/config.py | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) 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/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index 915cd381178e8..de4178d646b1f 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -200,8 +200,12 @@ class Chart extends React.Component { } } } else if ( - // chart should re-render if color scheme was changed - nextProps.formData?.color_scheme !== this.props.formData?.color_scheme + // 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; } diff --git a/superset/config.py b/superset/config.py index 43567c01c5ea2..27a9dc17cef0e 100644 --- a/superset/config.py +++ b/superset/config.py @@ -452,7 +452,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! From 7422e501180e3b79ca4a15615b78477f100c3598 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Tue, 4 Oct 2022 00:35:43 +0800 Subject: [PATCH 03/13] fix: getColor issue --- .../src/color/CategoricalColorScale.ts | 5 +- .../src/color/SharedLabelColorSingleton.ts | 58 ++++++++++--------- 2 files changed, 33 insertions(+), 30 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 e14011e3f026c..7597d5f003252 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -67,10 +67,11 @@ class CategoricalColorScale extends ExtensibleFunction { const cleanedValue = stringifyAndTrim(value); const sharedLabelColor = getSharedLabelColor(); - // priority: parentForcedColors > forcedColors + // priority: parentForcedColors > forcedColors > labelColors let color = this.parentForcedColors?.[cleanedValue] || - this.forcedColors?.[cleanedValue]; + this.forcedColors?.[cleanedValue] || + sharedLabelColor.allColorMap.get(cleanedValue); if (!color) { if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { 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 4ddd5b9150ddd..10e8b296552e2 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -21,59 +21,61 @@ import { CategoricalColorNamespace } from '.'; import { makeSingleton } from '../utils'; export class SharedLabelColor { - sliceLabelColorMap: Record>; + sliceLabelColorMap: Map>; + + allColorMap: Map; constructor() { // { sliceId1: { label1: color1 }, sliceId2: { label2: color2 } } - this.sliceLabelColorMap = {}; + this.sliceLabelColorMap = new Map(); + this.allColorMap = new Map(); } updateColorMap(colorNamespace?: string, colorScheme?: string) { const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); const colorScale = categoricalNamespace.getScale(colorScheme); - const newSliceLabelColorMap = {}; - Object.keys(this.sliceLabelColorMap).forEach(sliceId => { - Object.keys(this.sliceLabelColorMap[sliceId]).forEach(label => { - newSliceLabelColorMap[sliceId] = { - ...newSliceLabelColorMap[sliceId], - [label]: colorScale(label), - }; + const newSliceLabelColorMap = new Map(); + this.sliceLabelColorMap.forEach((colorMap, sliceId) => { + const newColorMap = new Map(); + colorMap.forEach((_, label) => { + const newColor = colorScale(label); + newColorMap.set(label, newColor); + this.allColorMap.set(label, newColor); }); + newSliceLabelColorMap.set(sliceId, newColorMap); }); this.sliceLabelColorMap = newSliceLabelColorMap; } getColorMap() { - return Object.keys(this.sliceLabelColorMap).reduce( - (res, sliceId) => ({ - ...res, - ...Object.keys(this.sliceLabelColorMap[sliceId]).reduce( - (res, label) => ({ - ...res, - [label]: this.sliceLabelColorMap[sliceId][label], - }), - {}, - ), - }), - {} as Record, - ); + return Object.fromEntries(this.allColorMap); } addSlice(label: string, color: string, sliceId?: number) { if (!sliceId) return; - this.sliceLabelColorMap[sliceId] = { - ...this.sliceLabelColorMap[sliceId], - [label]: color, - }; + let colorMap = this.sliceLabelColorMap.get(sliceId); + if (!colorMap) { + colorMap = new Map(); + } + colorMap.set(label, color); + this.sliceLabelColorMap.set(sliceId, colorMap); + this.allColorMap.set(label, color); } removeSlice(sliceId: number) { - delete this.sliceLabelColorMap[sliceId]; + const removeColorMap = this.sliceLabelColorMap.get(sliceId); + if (removeColorMap) { + removeColorMap.forEach((_, label) => { + this.allColorMap.delete(label); + }); + } + this.sliceLabelColorMap.delete(sliceId); } clear() { - this.sliceLabelColorMap = {}; + this.sliceLabelColorMap.clear(); + this.allColorMap.clear(); } } From 24b7e7b843a0f93fb6e5a56e33805fa6ef1d1dd5 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Tue, 4 Oct 2022 17:24:21 +0800 Subject: [PATCH 04/13] fix: test --- .../src/color/SharedLabelColorSingleton.ts | 10 +++++----- .../color/SharedLabelColorSingleton.test.ts | 17 +++++++++++------ 2 files changed, 16 insertions(+), 11 deletions(-) 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 10e8b296552e2..74560f37b1685 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -35,17 +35,17 @@ export class SharedLabelColor { const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); const colorScale = categoricalNamespace.getScale(colorScheme); - const newSliceLabelColorMap = new Map(); - this.sliceLabelColorMap.forEach((colorMap, sliceId) => { + const newSliceLabelColorMap = new Map(this.sliceLabelColorMap); + this.clear(); + newSliceLabelColorMap.forEach((colorMap, sliceId) => { const newColorMap = new Map(); colorMap.forEach((_, label) => { const newColor = colorScale(label); newColorMap.set(label, newColor); this.allColorMap.set(label, newColor); }); - newSliceLabelColorMap.set(sliceId, newColorMap); + this.sliceLabelColorMap.set(sliceId, newColorMap); }); - this.sliceLabelColorMap = newSliceLabelColorMap; } getColorMap() { @@ -53,7 +53,7 @@ export class SharedLabelColor { } addSlice(label: string, color: string, sliceId?: number) { - if (!sliceId) return; + if (sliceId === undefined) return; let colorMap = this.sliceLabelColorMap.get(sliceId); if (!colorMap) { colorMap = new Map(); 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 8aa97349b707f..39865f0ab99f0 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 @@ -60,18 +60,21 @@ 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.sliceLabelColorMap.has(1)).toEqual(true); + const colorMap = sharedLabelColor.sliceLabelColorMap.get(1); + expect(colorMap?.has('a')).toEqual(true); + expect(colorMap?.get('a')).toEqual('red'); }); it('should do nothing when sliceId is undefined', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red'); - expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( + {}, + ); }); }); @@ -80,7 +83,9 @@ describe('SharedLabelColor', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.removeSlice(1); - expect(sharedLabelColor.sliceLabelColorMap).toEqual({}); + expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( + {}, + ); }); }); From 6e79fac87ce951dfd3dbe84c9d42a6cf61c8a4b3 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Tue, 4 Oct 2022 23:08:23 +0800 Subject: [PATCH 05/13] fix: ci --- .../src/color/SharedLabelColorSingleton.ts | 9 +++++ .../superset-ui-core/src/color/index.ts | 1 + .../color/SharedLabelColorSingleton.test.ts | 40 +++++++++++++++++-- superset-frontend/src/explore/ExplorePage.tsx | 10 ++++- 4 files changed, 56 insertions(+), 4 deletions(-) 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 74560f37b1685..7977ab1343794 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -20,15 +20,22 @@ import { CategoricalColorNamespace } from '.'; import { makeSingleton } from '../utils'; +export enum SharedLabelColorSource { + dashboard, + explore, +} export class SharedLabelColor { sliceLabelColorMap: Map>; allColorMap: Map; + source: SharedLabelColorSource; + constructor() { // { sliceId1: { label1: color1 }, sliceId2: { label2: color2 } } this.sliceLabelColorMap = new Map(); this.allColorMap = new Map(); + this.source = SharedLabelColorSource.dashboard; } updateColorMap(colorNamespace?: string, colorScheme?: string) { @@ -53,6 +60,7 @@ export class SharedLabelColor { } addSlice(label: string, color: string, sliceId?: number) { + if (this.source !== SharedLabelColorSource.dashboard) return; if (sliceId === undefined) return; let colorMap = this.sliceLabelColorMap.get(sliceId); if (!colorMap) { @@ -64,6 +72,7 @@ export class SharedLabelColor { } removeSlice(sliceId: number) { + if (this.source !== SharedLabelColorSource.dashboard) return; const removeColorMap = this.sliceLabelColorMap.get(sliceId); if (removeColorMap) { removeColorMap.forEach((_, label) => { 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/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts index 39865f0ab99f0..1502855b96442 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(); }); @@ -69,6 +71,24 @@ describe('SharedLabelColor', () => { expect(colorMap?.get('a')).toEqual('red'); }); + it('should add to sliceLabelColorMap when label exist', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.addSlice('b', 'blue', 1); + const colorMap = sharedLabelColor.sliceLabelColorMap.get(1); + expect(colorMap?.has('b')).toEqual(true); + expect(colorMap?.get('b')).toEqual('blue'); + }); + + it('should do nothing when source is not dashboard', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.source = SharedLabelColorSource.explore; + sharedLabelColor.addSlice('a', 'red'); + expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( + {}, + ); + }); + it('should do nothing when sliceId is undefined', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red'); @@ -83,9 +103,23 @@ describe('SharedLabelColor', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.removeSlice(1); - expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( - {}, - ); + expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(false); + }); + + it('should do nothing when source is not dashboard', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.source = SharedLabelColorSource.explore; + sharedLabelColor.removeSlice(1); + expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); + }); + + it('should remove sliceId when remove map does not exist', () => { + const sharedLabelColor = getSharedLabelColor(); + sharedLabelColor.addSlice('a', 'red', 1); + sharedLabelColor.removeSlice(2); + expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); + expect(sharedLabelColor.sliceLabelColorMap.has(2)).toEqual(false); }); }); diff --git a/superset-frontend/src/explore/ExplorePage.tsx b/superset-frontend/src/explore/ExplorePage.tsx index 7f5be7e5e4002..e6de7369f857c 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'; @@ -137,6 +144,7 @@ export default function ExplorePage() { isExploreInitialized.current = true; }); } + getSharedLabelColor().source = SharedLabelColorSource.explore; }, [dispatch, location]); if (!isLoaded) { From d54aa7c1f5028cc89ce956bc0ff9160bc3285ac3 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Wed, 5 Oct 2022 00:02:35 +0800 Subject: [PATCH 06/13] fix: add chart color --- .../src/color/SharedLabelColorSingleton.ts | 19 ++++++++++--------- .../color/SharedLabelColorSingleton.test.ts | 8 -------- 2 files changed, 10 insertions(+), 17 deletions(-) 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 7977ab1343794..1c2ae3b95c39e 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -25,7 +25,7 @@ export enum SharedLabelColorSource { explore, } export class SharedLabelColor { - sliceLabelColorMap: Map>; + sliceLabelColorMap: Map>; allColorMap: Map; @@ -42,9 +42,10 @@ export class SharedLabelColor { const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); const colorScale = categoricalNamespace.getScale(colorScheme); - const newSliceLabelColorMap = new Map(this.sliceLabelColorMap); + colorScale.domain([...this.allColorMap.keys()]); + const copySliceLabelColorMap = new Map(this.sliceLabelColorMap); this.clear(); - newSliceLabelColorMap.forEach((colorMap, sliceId) => { + copySliceLabelColorMap.forEach((colorMap, sliceId) => { const newColorMap = new Map(); colorMap.forEach((_, label) => { const newColor = colorScale(label); @@ -56,6 +57,12 @@ export class SharedLabelColor { } getColorMap() { + this.allColorMap.clear(); + this.sliceLabelColorMap.forEach(colorMap => { + colorMap.forEach((color, label) => { + this.allColorMap.set(label, color); + }); + }); return Object.fromEntries(this.allColorMap); } @@ -73,12 +80,6 @@ export class SharedLabelColor { removeSlice(sliceId: number) { if (this.source !== SharedLabelColorSource.dashboard) return; - const removeColorMap = this.sliceLabelColorMap.get(sliceId); - if (removeColorMap) { - removeColorMap.forEach((_, label) => { - this.allColorMap.delete(label); - }); - } this.sliceLabelColorMap.delete(sliceId); } 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 1502855b96442..73b18f4ee8669 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 @@ -113,14 +113,6 @@ describe('SharedLabelColor', () => { sharedLabelColor.removeSlice(1); expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); }); - - it('should remove sliceId when remove map does not exist', () => { - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.removeSlice(2); - expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); - expect(sharedLabelColor.sliceLabelColorMap.has(2)).toEqual(false); - }); }); describe('.updateColorMap(namespace, scheme)', () => { From 1594becdfff7eb2ef60c98756a0c1755f4cb441b Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Sat, 8 Oct 2022 21:37:31 +0800 Subject: [PATCH 07/13] fix: color not reset if remove color scheme --- .../src/color/CategoricalColorScale.ts | 5 +- .../src/color/SharedLabelColorSingleton.ts | 73 ++++++++++--------- .../color/SharedLabelColorSingleton.test.ts | 52 ++++++++----- .../src/dashboard/actions/dashboardState.js | 29 -------- .../components/Header/Header.test.tsx | 3 +- .../src/dashboard/components/Header/index.jsx | 15 ++-- .../src/dashboard/components/Header/types.ts | 3 +- .../components/PropertiesModal/index.tsx | 24 +++--- .../components/gridComponents/ChartHolder.tsx | 11 --- .../containers/DashboardComponent.jsx | 2 - .../dashboard/containers/DashboardHeader.jsx | 6 +- .../src/dashboard/reducers/dashboardState.js | 9 --- .../dashboard/reducers/dashboardState.test.js | 9 +-- 13 files changed, 108 insertions(+), 133 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 7597d5f003252..817a522c1fefe 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -51,13 +51,14 @@ 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; this.colors = colors; this.scale = scaleOrdinal<{ toString(): string }, string>(); this.scale.range(colors); + this.scale.domain(Object.keys(parentForcedColors)); this.parentForcedColors = parentForcedColors; this.forcedColors = {}; this.multiple = 0; @@ -71,7 +72,7 @@ class CategoricalColorScale extends ExtensibleFunction { let color = this.parentForcedColors?.[cleanedValue] || this.forcedColors?.[cleanedValue] || - sharedLabelColor.allColorMap.get(cleanedValue); + sharedLabelColor.getColorMap().get(cleanedValue); if (!color) { if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { 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 1c2ae3b95c39e..e63caf155f48f 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -25,16 +25,16 @@ export enum SharedLabelColorSource { explore, } export class SharedLabelColor { - sliceLabelColorMap: Map>; + sliceLabelMap: Map; - allColorMap: Map; + colorMap: Map; source: SharedLabelColorSource; constructor() { - // { sliceId1: { label1: color1 }, sliceId2: { label2: color2 } } - this.sliceLabelColorMap = new Map(); - this.allColorMap = new Map(); + // { sliceId1: [label1, label2, ...], sliceId2: [label1, label2, ...] } + this.sliceLabelMap = new Map(); + this.colorMap = new Map(); this.source = SharedLabelColorSource.dashboard; } @@ -42,50 +42,53 @@ export class SharedLabelColor { const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); const colorScale = categoricalNamespace.getScale(colorScheme); - colorScale.domain([...this.allColorMap.keys()]); - const copySliceLabelColorMap = new Map(this.sliceLabelColorMap); - this.clear(); - copySliceLabelColorMap.forEach((colorMap, sliceId) => { - const newColorMap = new Map(); - colorMap.forEach((_, label) => { - const newColor = colorScale(label); - newColorMap.set(label, newColor); - this.allColorMap.set(label, newColor); - }); - this.sliceLabelColorMap.set(sliceId, newColorMap); + colorScale.domain([...this.colorMap.keys()]); + const copyColorMap = new Map(this.colorMap); + this.colorMap.clear(); + copyColorMap.forEach((_, label) => { + const newColor = colorScale(label); + this.colorMap.set(label, newColor); }); } getColorMap() { - this.allColorMap.clear(); - this.sliceLabelColorMap.forEach(colorMap => { - colorMap.forEach((color, label) => { - this.allColorMap.set(label, color); - }); - }); - return Object.fromEntries(this.allColorMap); + return this.colorMap; } addSlice(label: string, color: string, sliceId?: number) { - if (this.source !== SharedLabelColorSource.dashboard) return; - if (sliceId === undefined) return; - let colorMap = this.sliceLabelColorMap.get(sliceId); - if (!colorMap) { - colorMap = new Map(); - } - colorMap.set(label, color); - this.sliceLabelColorMap.set(sliceId, colorMap); - this.allColorMap.set(label, color); + if ( + this.source !== SharedLabelColorSource.dashboard || + sliceId === undefined + ) + return; + const labels = this.sliceLabelMap.get(sliceId) || []; + labels.push(label); + this.sliceLabelMap.set(sliceId, labels); + this.colorMap.set(label, color); } removeSlice(sliceId: number) { if (this.source !== SharedLabelColorSource.dashboard) return; - this.sliceLabelColorMap.delete(sliceId); + 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; + } + + reset() { + const copyColorMap = new Map(this.colorMap); + copyColorMap.forEach((_, label) => { + this.colorMap.set(label, ''); + }); } clear() { - this.sliceLabelColorMap.clear(); - this.allColorMap.clear(); + this.sliceLabelMap.clear(); + this.colorMap.clear(); } } 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 73b18f4ee8669..273a4ef093a2c 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 @@ -65,36 +65,30 @@ describe('SharedLabelColor', () => { it('should add to sliceLabelColorMap when first adding label', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); - const colorMap = sharedLabelColor.sliceLabelColorMap.get(1); - expect(colorMap?.has('a')).toEqual(true); - expect(colorMap?.get('a')).toEqual('red'); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(true); + const labels = sharedLabelColor.sliceLabelMap.get(1); + expect(labels?.includes('a')).toEqual(true); }); it('should add to sliceLabelColorMap when label exist', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.addSlice('b', 'blue', 1); - const colorMap = sharedLabelColor.sliceLabelColorMap.get(1); - expect(colorMap?.has('b')).toEqual(true); - expect(colorMap?.get('b')).toEqual('blue'); + const labels = sharedLabelColor.sliceLabelMap.get(1); + expect(labels?.includes('b')).toEqual(true); }); it('should do nothing when source is not dashboard', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.source = SharedLabelColorSource.explore; sharedLabelColor.addSlice('a', 'red'); - expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( - {}, - ); + expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); }); it('should do nothing when sliceId is undefined', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red'); - expect(Object.fromEntries(sharedLabelColor.sliceLabelColorMap)).toEqual( - {}, - ); + expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); }); }); @@ -103,7 +97,16 @@ describe('SharedLabelColor', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.removeSlice(1); - expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(false); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(false); + }); + + 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(Object.fromEntries(colorMap)).toEqual({ b: 'blue' }); }); it('should do nothing when source is not dashboard', () => { @@ -111,7 +114,7 @@ describe('SharedLabelColor', () => { sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.source = SharedLabelColorSource.explore; sharedLabelColor.removeSlice(1); - expect(sharedLabelColor.sliceLabelColorMap.has(1)).toEqual(true); + expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(true); }); }); @@ -122,7 +125,7 @@ describe('SharedLabelColor', () => { sharedLabelColor.addSlice('b', 'green', 2); sharedLabelColor.updateColorMap('', 'testColors2'); const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).toEqual({ a: 'yellow', b: 'green' }); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'yellow', b: 'green' }); }); it('should use recycle colors', () => { @@ -136,7 +139,7 @@ describe('SharedLabelColor', () => { sharedLabelColor.addSlice('d', 'red', 4); sharedLabelColor.updateColorMap('', 'testColors'); const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).not.toEqual({}); + expect(Object.fromEntries(colorMap)).not.toEqual({}); expect(getAnalogousColors).not.toBeCalled(); }); @@ -151,7 +154,7 @@ describe('SharedLabelColor', () => { sharedLabelColor.addSlice('d', 'red', 4); sharedLabelColor.updateColorMap('', 'testColors'); const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).not.toEqual({}); + expect(Object.fromEntries(colorMap)).not.toEqual({}); expect(getAnalogousColors).toBeCalled(); }); }); @@ -162,7 +165,18 @@ describe('SharedLabelColor', () => { sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.addSlice('b', 'blue', 2); const colorMap = sharedLabelColor.getColorMap(); - expect(colorMap).toEqual({ a: 'red', b: 'blue' }); + 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 b1fd92ea9fa0f..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,30 +501,6 @@ export function addSliceToDashboard(id, component) { }; } -export function postAddSliceFromDashboard() { - return (dispatch, getState) => { - const { - dashboardInfo: { metadata }, - dashboardState, - } = getState(); - const sharedLabelColor = getSharedLabelColor(); - - if (dashboardState?.updateSlice && dashboardState?.editMode) { - sharedLabelColor.updateColorMap( - metadata?.color_namespace, - metadata?.color_scheme, - ); - metadata.shared_label_colors = sharedLabelColor.getColorMap(); - dispatch( - dashboardInfoChanged({ - metadata, - }), - ); - dispatch(resetSlice()); - } - }; -} - export function removeSliceFromDashboard(id) { return (dispatch, getState) => { const sliceEntity = getState().sliceEntities.slices[id]; 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 c41126d9f5e7f..f5e9d9c6d87a9 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -74,8 +74,8 @@ const propTypes = { expandedSlices: PropTypes.object.isRequired, 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,7 +371,9 @@ class Header extends React.PureComponent { dashboardInfo?.metadata?.color_scheme || colorScheme; const currentColorNamespace = dashboardInfo?.metadata?.color_namespace || colorNamespace; - const currentSharedLabelColors = getSharedLabelColor().getColorMap(); + const currentSharedLabelColors = Object.fromEntries( + getSharedLabelColor().getColorMap(), + ); const data = { certified_by: dashboardInfo.certified_by, @@ -436,7 +438,8 @@ class Header extends React.PureComponent { customCss, colorNamespace, dataMask, - setColorSchemeAndUnsavedChanges, + setColorScheme, + setUnsavedChanges, colorScheme, onUndo, onRedo, @@ -477,6 +480,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 || '{}'), @@ -485,7 +490,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/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 7d46850f93dfa..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; @@ -266,7 +266,7 @@ const PropertiesModal = ({ }; const onColorSchemeChange = ( - colorScheme?: string, + colorScheme = '', { updateMetadata = true } = {}, ) => { // check that color_scheme is valid @@ -317,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) { @@ -328,13 +328,19 @@ const PropertiesModal = ({ } const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme); - metadata.shared_label_colors = sharedLabelColor.getColorMap(); - - if (metadata?.color_scheme) { + 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 = []; } @@ -363,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}`, @@ -381,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/ChartHolder.tsx b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx index c3cffe338c8e5..462d8f7654d08 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx +++ b/superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx @@ -66,7 +66,6 @@ interface ChartHolderProps { updateComponents: Function; handleComponentDrop: (...args: unknown[]) => unknown; setFullSizeChartId: (chartId: number | null) => void; - postAddSliceFromDashboard?: () => void; } const ChartHolder: React.FC = ({ @@ -90,7 +89,6 @@ const ChartHolder: React.FC = ({ updateComponents, handleComponentDrop, setFullSizeChartId, - postAddSliceFromDashboard, }) => { const { chartId } = component.meta; const isFullSize = fullSizeChartId === chartId; @@ -233,14 +231,6 @@ const ChartHolder: React.FC = ({ })); }, []); - const handlePostTransformProps = useCallback( - (props: unknown) => { - postAddSliceFromDashboard?.(); - return props; - }, - [postAddSliceFromDashboard], - ); - return ( = ({ isFullSize={isFullSize} setControlValue={handleExtraControl} extraControls={extraControls} - postTransformProps={handlePostTransformProps} /> {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/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( From f3212e457de01989e8306c73eff7a8839ba17a6b Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Sun, 9 Oct 2022 10:27:49 +0800 Subject: [PATCH 08/13] fix lint --- superset-frontend/src/dashboard/components/Header/index.jsx | 1 + 1 file changed, 1 insertion(+) diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index f5e9d9c6d87a9..178f6da6eeb43 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -74,6 +74,7 @@ const propTypes = { expandedSlices: PropTypes.object.isRequired, customCss: PropTypes.string.isRequired, colorNamespace: PropTypes.string, + colorScheme: PropTypes.string, setColorScheme: PropTypes.func.isRequired, setUnsavedChanges: PropTypes.func.isRequired, isStarred: PropTypes.bool.isRequired, From 37e8e802c1f513933dbb8fe05d269dc6ab85fd5f Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Mon, 10 Oct 2022 14:02:44 +0800 Subject: [PATCH 09/13] fix: test --- .../superset-ui-core/test/color/CategoricalColorScale.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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..601762a50869a 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: 'red', }); }); }); From a9bacb5fc2a426f14fef3d7eb3bc5b5e48f9f880 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Mon, 10 Oct 2022 17:27:01 +0800 Subject: [PATCH 10/13] fix: test --- .../components/PropertiesModal/PropertiesModal.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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: [], From 6db4c30ed5983f430d2df3cd6386a8047082b633 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Tue, 11 Oct 2022 01:26:03 +0800 Subject: [PATCH 11/13] fix --- .../src/color/CategoricalColorScale.ts | 1 - .../src/color/SharedLabelColorSingleton.ts | 14 ++++++++------ .../test/color/CategoricalColorScale.test.ts | 2 +- .../test/color/SharedLabelColorSingleton.test.ts | 14 ++++++++++---- 4 files changed, 19 insertions(+), 12 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 817a522c1fefe..c03c525198faf 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -58,7 +58,6 @@ class CategoricalColorScale extends ExtensibleFunction { this.colors = colors; this.scale = scaleOrdinal<{ toString(): string }, string>(); this.scale.range(colors); - this.scale.domain(Object.keys(parentForcedColors)); this.parentForcedColors = parentForcedColors; this.forcedColors = {}; this.multiple = 0; 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 e63caf155f48f..c4dbf459542a6 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -41,14 +41,16 @@ export class SharedLabelColor { updateColorMap(colorNamespace?: string, colorScheme?: string) { const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); - const colorScale = categoricalNamespace.getScale(colorScheme); - colorScale.domain([...this.colorMap.keys()]); - const copyColorMap = new Map(this.colorMap); + const newColorMap = new Map(); this.colorMap.clear(); - copyColorMap.forEach((_, label) => { - const newColor = colorScale(label); - this.colorMap.set(label, newColor); + this.sliceLabelMap.forEach(labels => { + const colorScale = categoricalNamespace.getScale(colorScheme); + labels.forEach(label => { + const newColor = colorScale(label); + newColorMap.set(label, newColor); + }); }); + this.colorMap = newColorMap; } getColorMap() { 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 601762a50869a..91a8f4a3185a7 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: 'red', + horse: 'blue', }); }); }); 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 273a4ef093a2c..2441f886f3ac4 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 @@ -122,10 +122,16 @@ describe('SharedLabelColor', () => { it('should update color map', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); + 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: 'green' }); + expect(Object.fromEntries(colorMap)).toEqual({ + a: 'yellow', + b: 'yellow', + c: 'green', + }); }); it('should use recycle colors', () => { @@ -149,9 +155,9 @@ describe('SharedLabelColor', () => { }; const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.addSlice('b', 'blue', 2); - sharedLabelColor.addSlice('c', 'green', 3); - sharedLabelColor.addSlice('d', 'red', 4); + 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({}); From c9c851c36883e5956771dafa278046ed99f99883 Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Thu, 13 Oct 2022 15:41:32 +0800 Subject: [PATCH 12/13] fix: color source --- .../src/dashboard/containers/DashboardPage.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index 17097b6c1c800..e40c907e95ed3 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -22,6 +22,7 @@ import { FeatureFlag, getSharedLabelColor, isFeatureEnabled, + SharedLabelColorSource, t, useTheme, } from '@superset-ui/core'; @@ -333,17 +334,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) { From d20665a1879692db892b22523cf481b1e574b33b Mon Sep 17 00:00:00 2001 From: stephenLYZ <750188453@qq.com> Date: Fri, 14 Oct 2022 13:25:26 +0800 Subject: [PATCH 13/13] fix label color will change color order --- .../src/color/CategoricalColorScale.ts | 23 ++++++++++--------- .../src/color/SharedLabelColorSingleton.ts | 6 +++-- .../test/color/CategoricalColorScale.test.ts | 2 +- .../color/SharedLabelColorSingleton.test.ts | 17 +++++++++++++- 4 files changed, 33 insertions(+), 15 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 c03c525198faf..5f29ad4775996 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -73,18 +73,19 @@ class CategoricalColorScale extends ExtensibleFunction { this.forcedColors?.[cleanedValue] || sharedLabelColor.getColorMap().get(cleanedValue); - if (!color) { - if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { - const multiple = Math.floor( - this.domain().length / this.originColors.length, - ); - if (multiple > this.multiple) { - this.multiple = multiple; - const newRange = getAnalogousColors(this.originColors, multiple); - this.range(this.originColors.concat(newRange)); - } + if (isFeatureEnabled(FeatureFlag.USE_ANALAGOUS_COLORS)) { + const multiple = Math.floor( + this.domain().length / this.originColors.length, + ); + if (multiple > this.multiple) { + this.multiple = multiple; + const newRange = getAnalogousColors(this.originColors, multiple); + this.range(this.originColors.concat(newRange)); } - color = this.scale(cleanedValue); + } + const newColor = this.scale(cleanedValue); + if (!color) { + color = newColor; } sharedLabelColor.addSlice(cleanedValue, color, sliceId); 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 c4dbf459542a6..bc417e6036a51 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -64,8 +64,10 @@ export class SharedLabelColor { ) return; const labels = this.sliceLabelMap.get(sliceId) || []; - labels.push(label); - this.sliceLabelMap.set(sliceId, labels); + if (!labels.includes(label)) { + labels.push(label); + this.sliceLabelMap.set(sliceId, labels); + } this.colorMap.set(label, color); } 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 2441f886f3ac4..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 @@ -68,14 +68,29 @@ describe('SharedLabelColor', () => { 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 label exist', () => { + 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', () => {