From c54aa0874490e41f0df6e421e6e51430f3eb3c89 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Tue, 2 Nov 2021 15:16:49 +0200 Subject: [PATCH] fix: Color consistency (#1406) * Replace color in scheme statically * Set color statically and re-use instance * Fix tests and clean up * Improve comments * Refactor and simplify * Update tests * Remove unnecessary ColorMapControl * Remove unnecessary const * Remove control label_colors --- .../src/sections/sections.tsx | 2 +- .../src/shared-controls/index.tsx | 14 -------------- .../superset-ui-chart-controls/src/types.ts | 1 - .../src/color/CategoricalColorNamespace.ts | 14 ++++++++------ .../src/color/CategoricalColorScale.ts | 2 -- .../color/CategoricalColorNameSpace.test.ts | 19 +++---------------- .../src/controlPanel.ts | 5 +---- .../src/controlPanel.ts | 2 +- .../src/controlPanel.tsx | 2 +- .../src/controlPanel.tsx | 2 +- .../src/controlPanel.ts | 2 +- .../src/controlPanel.ts | 2 +- .../src/controlPanel.ts | 2 +- .../src/Area/controlPanel.ts | 2 +- .../src/Bar/controlPanel.ts | 1 - .../src/Bubble/controlPanel.ts | 5 +---- .../src/Compare/controlPanel.ts | 2 +- .../src/DistBar/controlPanel.ts | 1 - .../src/DualLine/controlPanel.ts | 2 +- .../src/Line/controlPanel.ts | 1 - .../src/LineMulti/controlPanel.ts | 2 +- .../src/Pie/controlPanel.ts | 2 +- .../src/MixedTimeseries/controlPanel.tsx | 2 +- .../src/Timeseries/Area/controlPanel.tsx | 2 +- .../Regular/Scatter/controlPanel.tsx | 2 +- .../src/Timeseries/Regular/controlPanel.tsx | 2 +- .../src/Timeseries/Step/controlPanel.tsx | 2 +- .../src/Timeseries/controlPanel.tsx | 2 +- .../src/plugin/controlPanel.ts | 2 +- .../src/BoxPlot/controlPanel.ts | 2 +- 30 files changed, 33 insertions(+), 70 deletions(-) diff --git a/packages/superset-ui-chart-controls/src/sections/sections.tsx b/packages/superset-ui-chart-controls/src/sections/sections.tsx index eb93c469df..53ac2ae2ff 100644 --- a/packages/superset-ui-chart-controls/src/sections/sections.tsx +++ b/packages/superset-ui-chart-controls/src/sections/sections.tsx @@ -104,7 +104,7 @@ export const datasourceAndVizType: ControlPanelSectionConfig = { export const colorScheme: ControlPanelSectionConfig = { label: t('Color Scheme'), - controlSetRows: [['color_scheme', 'label_colors']], + controlSetRows: [['color_scheme']], }; export const annotations: ControlPanelSectionConfig = { diff --git a/packages/superset-ui-chart-controls/src/shared-controls/index.tsx b/packages/superset-ui-chart-controls/src/shared-controls/index.tsx index a95fe90c6f..3847a7afc8 100644 --- a/packages/superset-ui-chart-controls/src/shared-controls/index.tsx +++ b/packages/superset-ui-chart-controls/src/shared-controls/index.tsx @@ -478,19 +478,6 @@ const color_scheme: SharedControlConfig<'ColorSchemeControl'> = { schemes: () => categoricalSchemeRegistry.getMap(), }; -const label_colors: SharedControlConfig<'ColorMapControl'> = { - type: 'ColorMapControl', - label: t('Color Map'), - default: {}, - renderTrigger: true, - mapStateToProps: ({ - form_data: { color_namespace: colorNamespace, color_scheme: colorScheme }, - }) => ({ - colorNamespace, - colorScheme, - }), -}; - const enableExploreDnd = isFeatureEnabled(FeatureFlag.ENABLE_EXPLORE_DRAG_AND_DROP); const sharedControls = { @@ -522,7 +509,6 @@ const sharedControls = { x_axis_time_format, adhoc_filters: enableExploreDnd ? dnd_adhoc_filters : adhoc_filters, color_scheme, - label_colors, series_columns: enableExploreDnd ? dndColumnsControl : columnsControl, series_limit, series_limit_metric: enableExploreDnd ? dnd_sort_by : sort_by, diff --git a/packages/superset-ui-chart-controls/src/types.ts b/packages/superset-ui-chart-controls/src/types.ts index ca49316405..acaf6051f9 100644 --- a/packages/superset-ui-chart-controls/src/types.ts +++ b/packages/superset-ui-chart-controls/src/types.ts @@ -117,7 +117,6 @@ export type InternalControlType = | 'BoundsControl' | 'CheckboxControl' | 'CollectionControl' - | 'ColorMapControl' | 'ColorPickerControl' | 'ColorSchemeControl' | 'DatasourceControl' diff --git a/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts b/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts index f41c3f611c..220ffeea29 100644 --- a/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts +++ b/packages/superset-ui-core/src/color/CategoricalColorNamespace.ts @@ -39,14 +39,8 @@ export default class CategoricalColorNamespace { getScale(schemeId?: string) { const id = schemeId ?? getCategoricalSchemeRegistry().getDefaultKey() ?? ''; - const scale = this.scales[id]; - if (scale) { - return scale; - } const scheme = getCategoricalSchemeRegistry().get(id); - const newScale = new CategoricalColorScale(scheme?.colors ?? [], this.forcedItems); - this.scales[id] = newScale; return newScale; } @@ -63,6 +57,10 @@ export default class CategoricalColorNamespace { return this; } + + resetColors() { + this.forcedItems = {}; + } } const namespaces: { @@ -86,6 +84,10 @@ export function getColor(value?: string, schemeId?: string, namespace?: string) return getNamespace(namespace).getScale(schemeId).getColor(value); } +/* + Returns a new scale instance within the same namespace. + Especially useful when a chart is booting for the first time +*/ export function getScale(scheme?: string, namespace?: string) { return getNamespace(namespace).getScale(scheme); } diff --git a/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/packages/superset-ui-core/src/color/CategoricalColorScale.ts index 3e3ac88471..3044c99279 100644 --- a/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -57,7 +57,6 @@ class CategoricalColorScale extends ExtensibleFunction { getColor(value?: string) { const cleanedValue = stringifyAndTrim(value); - const parentColor = this.parentForcedColors && this.parentForcedColors[cleanedValue]; if (parentColor) { return parentColor; @@ -78,7 +77,6 @@ class CategoricalColorScale extends ExtensibleFunction { */ setColor(value: string, forcedColor: string) { this.forcedColors[stringifyAndTrim(value)] = forcedColor; - return this; } diff --git a/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts b/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts index 5349f2a14d..ff365ba78a 100644 --- a/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts +++ b/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts @@ -81,15 +81,6 @@ describe('CategoricalColorNamespace', () => { expect(scale).toBeDefined(); getCategoricalSchemeRegistry().setDefaultKey('testColors'); }); - it('returns same scale if the scale with that name already exists in this namespace', () => { - const namespace = getNamespace('test-get-scale2'); - const scale1 = namespace.getScale('testColors'); - const scale2 = namespace.getScale('testColors2'); - const scale3 = namespace.getScale('testColors2'); - const scale4 = namespace.getScale('testColors'); - expect(scale1).toBe(scale4); - expect(scale2).toBe(scale3); - }); }); describe('.setColor()', () => { it('overwrites color for all CategoricalColorScales in this namespace', () => { @@ -127,19 +118,15 @@ describe('CategoricalColorNamespace', () => { const scale = getScale(); expect(scale).toBeDefined(); const scale2 = getNamespace().getScale(); - expect(scale).toBe(scale2); + expect(scale2).toBeDefined(); }); it('getScale(scheme) returns a CategoricalColorScale with specified scheme in default namespace', () => { - const scale = getScale('testColors'); + const scale = getNamespace().getScale('testColors'); expect(scale).toBeDefined(); - const scale2 = getNamespace().getScale('testColors'); - expect(scale).toBe(scale2); }); it('getScale(scheme, namespace) returns a CategoricalColorScale with specified scheme in specified namespace', () => { - const scale = getScale('testColors', 'test-getScale'); + const scale = getNamespace('test-getScale').getScale('testColors'); expect(scale).toBeDefined(); - const scale2 = getNamespace('test-getScale').getScale('testColors'); - expect(scale).toBe(scale2); }); }); describe('static getColor()', () => { diff --git a/plugins/legacy-plugin-chart-chord/src/controlPanel.ts b/plugins/legacy-plugin-chart-chord/src/controlPanel.ts index c37332e1e7..73ae97f225 100644 --- a/plugins/legacy-plugin-chart-chord/src/controlPanel.ts +++ b/plugins/legacy-plugin-chart-chord/src/controlPanel.ts @@ -46,10 +46,7 @@ const config: ControlPanelConfig = { { label: t('Chart Options'), expanded: true, - controlSetRows: [ - ['y_axis_format', null], - ['color_scheme', 'label_colors'], - ], + controlSetRows: [['y_axis_format', null], ['color_scheme']], }, ], controlOverrides: { diff --git a/plugins/legacy-plugin-chart-histogram/src/controlPanel.ts b/plugins/legacy-plugin-chart-histogram/src/controlPanel.ts index 2dd936d5d6..b2de53cbc8 100644 --- a/plugins/legacy-plugin-chart-histogram/src/controlPanel.ts +++ b/plugins/legacy-plugin-chart-histogram/src/controlPanel.ts @@ -70,7 +70,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'link_length', diff --git a/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx b/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx index 8c21396035..7da0d93e3a 100644 --- a/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx +++ b/plugins/legacy-plugin-chart-partition/src/controlPanel.tsx @@ -131,7 +131,7 @@ const config: ControlPanelConfig = { expanded: true, tabOverride: 'customize', controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'number_format', diff --git a/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx b/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx index 960ce13726..d4dde69341 100644 --- a/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx +++ b/plugins/legacy-plugin-chart-rose/src/controlPanel.tsx @@ -65,7 +65,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'number_format', diff --git a/plugins/legacy-plugin-chart-sankey-loop/src/controlPanel.ts b/plugins/legacy-plugin-chart-sankey-loop/src/controlPanel.ts index f659aa51a5..0b2c8a0e55 100644 --- a/plugins/legacy-plugin-chart-sankey-loop/src/controlPanel.ts +++ b/plugins/legacy-plugin-chart-sankey-loop/src/controlPanel.ts @@ -30,7 +30,7 @@ const config: ControlPanelConfig = { { label: t('Chart Options'), expanded: true, - controlSetRows: [['color_scheme', 'label_colors']], + controlSetRows: [['color_scheme']], }, ], controlOverrides: { diff --git a/plugins/legacy-plugin-chart-sankey/src/controlPanel.ts b/plugins/legacy-plugin-chart-sankey/src/controlPanel.ts index 1386d45500..7c7a2a5db9 100644 --- a/plugins/legacy-plugin-chart-sankey/src/controlPanel.ts +++ b/plugins/legacy-plugin-chart-sankey/src/controlPanel.ts @@ -62,7 +62,7 @@ const config: ControlPanelConfig = { { label: t('Chart Options'), expanded: true, - controlSetRows: [['color_scheme', 'label_colors']], + controlSetRows: [['color_scheme']], }, ], }; diff --git a/plugins/legacy-plugin-chart-treemap/src/controlPanel.ts b/plugins/legacy-plugin-chart-treemap/src/controlPanel.ts index 24487dc238..9726396013 100644 --- a/plugins/legacy-plugin-chart-treemap/src/controlPanel.ts +++ b/plugins/legacy-plugin-chart-treemap/src/controlPanel.ts @@ -54,7 +54,7 @@ const config: ControlPanelConfig = { expanded: true, tabOverride: 'customize', controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'treemap_ratio', diff --git a/plugins/legacy-preset-chart-nvd3/src/Area/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Area/controlPanel.ts index 204770ff74..c3c08a23c0 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Area/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Area/controlPanel.ts @@ -61,7 +61,7 @@ const config: ControlPanelConfig = { }, }, ], - ['color_scheme', 'label_colors'], + ['color_scheme'], [richTooltip, showControls], ], }, diff --git a/plugins/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts index c9e6595baa..d53ad9fcf5 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts @@ -49,7 +49,6 @@ const config: ControlPanelConfig = { expanded: true, controlSetRows: [ ['color_scheme'], - ['label_colors'], [showBrush], [showLegend], [showBarValue], diff --git a/plugins/legacy-preset-chart-nvd3/src/Bubble/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Bubble/controlPanel.ts index acf001e823..5dc3e97b30 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Bubble/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Bubble/controlPanel.ts @@ -69,10 +69,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, tabOverride: 'customize', - controlSetRows: [ - ['color_scheme', 'label_colors'], - [showLegend, null], - ], + controlSetRows: [['color_scheme'], [showLegend, null]], }, { label: t('X Axis'), diff --git a/plugins/legacy-preset-chart-nvd3/src/Compare/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Compare/controlPanel.ts index 0a68fe8e44..db4a84fd47 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Compare/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Compare/controlPanel.ts @@ -39,7 +39,7 @@ const config: ControlPanelConfig = { { label: t('Chart Options'), expanded: true, - controlSetRows: [['color_scheme', 'label_colors']], + controlSetRows: [['color_scheme']], }, { label: t('X Axis'), diff --git a/plugins/legacy-preset-chart-nvd3/src/DistBar/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/DistBar/controlPanel.ts index 2adf7376d8..923a2bfc05 100644 --- a/plugins/legacy-preset-chart-nvd3/src/DistBar/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/DistBar/controlPanel.ts @@ -79,7 +79,6 @@ const config: ControlPanelConfig = { expanded: true, controlSetRows: [ ['color_scheme'], - ['label_colors'], [showLegend], [showBarValue], [barStacked], diff --git a/plugins/legacy-preset-chart-nvd3/src/DualLine/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/DualLine/controlPanel.ts index 8173536122..1c01cb4758 100644 --- a/plugins/legacy-preset-chart-nvd3/src/DualLine/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/DualLine/controlPanel.ts @@ -34,7 +34,7 @@ const config: ControlPanelConfig = { { label: t('Chart Options'), expanded: true, - controlSetRows: [['color_scheme', 'label_colors'], [showLegend], [xAxisFormat]], + controlSetRows: [['color_scheme'], [showLegend], [xAxisFormat]], }, { label: t('Y Axis 1'), diff --git a/plugins/legacy-preset-chart-nvd3/src/Line/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Line/controlPanel.ts index 7815375ec9..5b277cf671 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Line/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Line/controlPanel.ts @@ -46,7 +46,6 @@ const config: ControlPanelConfig = { expanded: true, controlSetRows: [ ['color_scheme'], - ['label_colors'], [showBrush], [ { diff --git a/plugins/legacy-preset-chart-nvd3/src/LineMulti/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/LineMulti/controlPanel.ts index f6b9c7d8cb..488b020198 100644 --- a/plugins/legacy-preset-chart-nvd3/src/LineMulti/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/LineMulti/controlPanel.ts @@ -55,7 +55,7 @@ const config: ControlPanelConfig = { tabOverride: 'customize', expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'prefix_metric_with_slice_name', diff --git a/plugins/legacy-preset-chart-nvd3/src/Pie/controlPanel.ts b/plugins/legacy-preset-chart-nvd3/src/Pie/controlPanel.ts index 9cf3232270..05baeb63d9 100644 --- a/plugins/legacy-preset-chart-nvd3/src/Pie/controlPanel.ts +++ b/plugins/legacy-preset-chart-nvd3/src/Pie/controlPanel.ts @@ -103,7 +103,7 @@ const config: ControlPanelConfig = { }, }, ], - ['color_scheme', 'label_colors'], + ['color_scheme'], ], }, ], diff --git a/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx b/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx index d30339185b..ebe479f12d 100644 --- a/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx @@ -265,7 +265,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], ...createCustomizeSection(t('Query A'), ''), ...createCustomizeSection(t('Query B'), 'B'), [ diff --git a/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx b/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx index adabeecd2e..b58f3d0c14 100644 --- a/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx @@ -99,7 +99,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'seriesType', diff --git a/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx b/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx index 8754d3733b..33c99b0e77 100644 --- a/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx @@ -76,7 +76,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], ...showValueSection, [ { diff --git a/plugins/plugin-chart-echarts/src/Timeseries/Regular/controlPanel.tsx b/plugins/plugin-chart-echarts/src/Timeseries/Regular/controlPanel.tsx index 818036207f..3caee6194f 100644 --- a/plugins/plugin-chart-echarts/src/Timeseries/Regular/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/Timeseries/Regular/controlPanel.tsx @@ -93,7 +93,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], ...showValueSection, [ { diff --git a/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx b/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx index 4a713931d7..adf39a564d 100644 --- a/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/Timeseries/Step/controlPanel.tsx @@ -99,7 +99,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'seriesType', diff --git a/plugins/plugin-chart-echarts/src/Timeseries/controlPanel.tsx b/plugins/plugin-chart-echarts/src/Timeseries/controlPanel.tsx index 68f1a3f459..7cd89bdf2a 100644 --- a/plugins/plugin-chart-echarts/src/Timeseries/controlPanel.tsx +++ b/plugins/plugin-chart-echarts/src/Timeseries/controlPanel.tsx @@ -100,7 +100,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'seriesType', diff --git a/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.ts b/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.ts index 715d9eb120..6deb658ebb 100644 --- a/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.ts +++ b/plugins/plugin-chart-word-cloud/src/plugin/controlPanel.ts @@ -88,7 +88,7 @@ const config: ControlPanelConfig = { }, }, ], - ['color_scheme', 'label_colors'], + ['color_scheme'], ], }, ], diff --git a/plugins/preset-chart-xy/src/BoxPlot/controlPanel.ts b/plugins/preset-chart-xy/src/BoxPlot/controlPanel.ts index 6ed2d2f970..599390baae 100644 --- a/plugins/preset-chart-xy/src/BoxPlot/controlPanel.ts +++ b/plugins/preset-chart-xy/src/BoxPlot/controlPanel.ts @@ -31,7 +31,7 @@ const config: ControlPanelConfig = { label: t('Chart Options'), expanded: true, controlSetRows: [ - ['color_scheme', 'label_colors'], + ['color_scheme'], [ { name: 'whisker_options',