From 313ee596f5435894f857d72be7269d5070c8c964 Mon Sep 17 00:00:00 2001 From: Geido <60598000+geido@users.noreply.github.com> Date: Thu, 20 Jun 2024 15:30:11 +0200 Subject: [PATCH] fix(Dashboard): Color inconsistency on refreshes and conflicts (#27439) --- RESOURCES/FEATURE_FLAGS.md | 1 + .../cypress/e2e/dashboard/editmode.test.ts | 49 ++- .../explore/visualizations/compare.test.js | 3 - .../explore/visualizations/dist_bar.test.js | 3 - .../src/color/CategoricalColorScale.ts | 219 +++++++++---- ...ingleton.ts => LabelsColorMapSingleton.ts} | 74 +++-- .../superset-ui-core/src/color/index.ts | 8 +- .../superset-ui-core/src/color/utils.ts | 5 +- .../useComponentDidUpdate.ts | 10 +- .../color/CategoricalColorNameSpace.test.ts | 2 +- .../test/color/CategoricalColorScale.test.ts | 290 ++++++++++++------ .../color/LabelsColorMapSingleton.test.ts | 234 ++++++++++++++ .../color/SharedLabelColorSingleton.test.ts | 201 ------------ .../legacy-plugin-chart-chord/src/Chord.js | 4 +- .../src/Histogram.jsx | 2 +- .../src/Partition.js | 2 +- .../legacy-plugin-chart-rose/src/Rose.js | 14 +- .../legacy-plugin-chart-sankey/src/Sankey.js | 2 +- .../src/CategoricalDeckGLContainer.tsx | 10 +- .../legacy-preset-chart-nvd3/src/NVD3Vis.js | 4 +- .../src/BoxPlot/transformProps.ts | 6 +- .../src/Funnel/transformProps.ts | 2 +- .../src/Gauge/transformProps.ts | 4 +- .../src/Graph/transformProps.ts | 2 +- .../src/Pie/transformProps.ts | 2 +- .../src/Radar/transformProps.ts | 2 +- .../src/Treemap/transformProps.ts | 4 +- .../src/chart/WordCloud.tsx | 9 +- .../src/legacyPlugin/transformProps.ts | 1 + .../src/plugin/transformProps.ts | 4 +- .../test/legacyPlugin/transformProps.test.ts | 1 + .../src/components/Chart/Chart.jsx | 4 +- .../src/components/Chart/ChartRenderer.jsx | 8 +- .../src/dashboard/actions/dashboardInfo.ts | 39 +-- .../src/dashboard/actions/dashboardState.js | 80 ++++- .../src/dashboard/actions/hydrate.js | 11 - .../components/ColorSchemeControlWrapper.jsx | 8 +- .../DashboardBuilder/DashboardContainer.tsx | 118 ++----- .../src/dashboard/components/Header/index.jsx | 9 +- .../components/PropertiesModal/index.tsx | 36 +-- .../components/SyncDashboardState/index.tsx | 4 +- .../components/gridComponents/Chart.jsx | 12 +- .../components/gridComponents/Tabs.jsx | 1 + .../src/dashboard/containers/Chart.jsx | 12 +- .../containers/DashboardComponent.jsx | 1 + .../dashboard/containers/DashboardPage.tsx | 23 +- .../charts/getFormDataWithExtraFilters.ts | 16 +- .../components/ExploreChartHeader/index.jsx | 38 +-- .../ColorSchemeControl.test.tsx | 13 +- .../ColorSchemeControl/ColorSchemeLabel.tsx | 12 +- .../controls/ColorSchemeControl/index.tsx | 14 +- superset-frontend/src/pages/Chart/index.tsx | 14 +- .../src/types/DashboardContextForExplore.ts | 4 +- superset-frontend/src/utils/colorScheme.ts | 140 +++++++++ .../integration_tests/superset_test_config.py | 1 + 55 files changed, 1050 insertions(+), 742 deletions(-) rename superset-frontend/packages/superset-ui-core/src/color/{SharedLabelColorSingleton.ts => LabelsColorMapSingleton.ts} (52%) create mode 100644 superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts delete mode 100644 superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts create mode 100644 superset-frontend/src/utils/colorScheme.ts diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 950e652efa..2c4d4d0c3e 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -89,6 +89,7 @@ These features flags currently default to True and **will be removed in a future [//]: # "PLEASE KEEP THE LIST SORTED ALPHABETICALLY" +- AVOID_COLORS_COLLISION - DASHBOARD_CROSS_FILTERS - ENABLE_JAVASCRIPT_CONTROLS - KV_STORE diff --git a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts index 1584cd04e3..4f9863071c 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -124,7 +124,7 @@ function selectColorScheme(color: string) { ) .first() .click(); - cy.getBySel(color).click(); + cy.getBySel(color).click({ force: true }); } function applyChanges() { @@ -169,6 +169,7 @@ function writeMetadata(metadata: string) { function openExplore(chartName: string) { interceptExploreJson(); + interceptGet(); cy.get( `[data-test-chart-name='${chartName}'] [aria-label='More Options']`, @@ -210,7 +211,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(69, 78, 124)'); + .should('have.css', 'fill', 'rgb(31, 168, 201)'); }); it('should apply same color to same labels with color scheme set', () => { @@ -231,7 +232,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(0, 234, 162)'); + .should('have.css', 'fill', 'rgb(50, 0, 167)'); // open 2nd main tab openTab(0, 1); @@ -240,7 +241,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(0, 234, 162)'); + .should('have.css', 'fill', 'rgb(50, 0, 167)'); }); it('should apply same color to same labels with no color scheme set', () => { @@ -261,7 +262,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(69, 78, 124)'); + .should('have.css', 'fill', 'rgb(31, 168, 201)'); // open 2nd main tab openTab(0, 1); @@ -270,7 +271,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(69, 78, 124)'); + .should('have.css', 'fill', 'rgb(31, 168, 201)'); }); it('custom label colors should take the precedence in nested tabs', () => { @@ -384,17 +385,17 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(69, 78, 124)'); + .should('have.css', 'fill', 'rgb(31, 168, 201)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(1) - .should('have.css', 'fill', 'rgb(224, 67, 85)'); + .should('have.css', 'fill', 'rgb(69, 78, 124)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(163, 143, 121)'); + .should('have.css', 'fill', 'rgb(90, 193, 137)'); openProperties(); cy.get('[aria-label="Select color scheme"]').should('have.value', ''); @@ -423,17 +424,17 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(69, 78, 124)'); + .should('have.css', 'fill', 'rgb(31, 168, 201)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(1) - .should('have.css', 'fill', 'rgb(224, 67, 85)'); + .should('have.css', 'fill', 'rgb(69, 78, 124)'); cy.get( '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .eq(2) - .should('have.css', 'fill', 'rgb(163, 143, 121)'); + .should('have.css', 'fill', 'rgb(90, 193, 137)'); }); it('should show the same colors in Explore', () => { @@ -459,12 +460,6 @@ describe('Dashboard edit', () => { ) .first() .should('have.css', 'fill', 'rgb(255, 0, 0)'); - // label Christopher - cy.get( - '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', - ) - .eq(1) - .should('have.css', 'fill', 'rgb(172, 32, 119)'); openExplore('Top 10 California Names Timeseries'); @@ -472,10 +467,6 @@ describe('Dashboard edit', () => { cy.get('[data-test="chart-container"] .line .nv-legend-symbol') .first() .should('have.css', 'fill', 'rgb(255, 0, 0)'); - // label Christopher - cy.get('[data-test="chart-container"] .line .nv-legend-symbol') - .eq(1) - .should('have.css', 'fill', 'rgb(172, 32, 119)'); }); it.skip('should change color scheme multiple times', () => { @@ -496,7 +487,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); // open 2nd main tab openTab(0, 1); @@ -505,7 +496,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); editDashboard(); openProperties(); @@ -516,7 +507,7 @@ describe('Dashboard edit', () => { // label Anthony cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .eq(2) - .should('have.css', 'fill', 'rgb(244, 176, 42)'); + .should('have.css', 'fill', 'rgb(41, 105, 107)'); // open main tab and nested tab openTab(0, 0); @@ -527,7 +518,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(244, 176, 42)'); + .should('have.css', 'fill', 'rgb(41, 105, 107)'); }); it.skip('should apply the color scheme across main tabs', () => { @@ -542,7 +533,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); }); it.skip('should apply the color scheme across main tabs for rendered charts', () => { @@ -558,7 +549,7 @@ describe('Dashboard edit', () => { cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol') .first() - .should('have.css', 'fill', 'rgb(156, 52, 152)'); + .should('have.css', 'fill', 'rgb(41, 105, 107)'); // change scheme now that charts are rendered across the main tabs editDashboard(); @@ -588,7 +579,7 @@ describe('Dashboard edit', () => { '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol', ) .first() - .should('have.css', 'fill', 'rgb(51, 61, 71)'); + .should('have.css', 'fill', 'rgb(234, 11, 140)'); // open another nested tab openTab(2, 1); diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/compare.test.js b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/compare.test.js index 136e48d5ad..11ad6eb578 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/compare.test.js +++ b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/compare.test.js @@ -97,8 +97,5 @@ describe('Visualization > Compare', () => { cy.get( '.Control[data-test="color_scheme"] .ant-select-selection-item [data-test="supersetColors"]', ).should('exist'); - cy.get('.compare .nv-legend .nv-legend-symbol') - .first() - .should('have.css', 'fill', 'rgb(31, 168, 201)'); }); }); diff --git a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/dist_bar.test.js b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/dist_bar.test.js index 591ba31776..8dcba5b755 100644 --- a/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/dist_bar.test.js +++ b/superset-frontend/cypress-base/cypress/e2e/explore/visualizations/dist_bar.test.js @@ -87,8 +87,5 @@ describe('Visualization > Distribution bar chart', () => { cy.get( '.Control[data-test="color_scheme"] .ant-select-selection-item [data-test="bnbColors"]', ).should('exist'); - cy.get('.dist_bar .nv-legend .nv-legend-symbol') - .first() - .should('have.css', 'fill', 'rgb(41, 105, 107)'); }); }); 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 8b8cd02519..f97f84cdec 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -17,19 +17,18 @@ * under the License. */ -/* eslint-disable no-dupe-class-members */ import { scaleOrdinal, ScaleOrdinal } from 'd3-scale'; import { ExtensibleFunction } from '../models'; import { ColorsInitLookup, ColorsLookup } from './types'; import stringifyAndTrim from './stringifyAndTrim'; -import getSharedLabelColor from './SharedLabelColorSingleton'; +import getLabelsColorMap from './LabelsColorMapSingleton'; import { getAnalogousColors } from './utils'; import { FeatureFlag, isFeatureEnabled } from '../utils'; // Use type augmentation to correct the fact that // an instance of CategoricalScale is also a function interface CategoricalColorScale { - (x: { toString(): string }, y?: number): string; + (x: { toString(): string }, y?: number, w?: string): string; } class CategoricalColorScale extends ExtensibleFunction { @@ -39,101 +38,183 @@ class CategoricalColorScale extends ExtensibleFunction { scale: ScaleOrdinal<{ toString(): string }, string>; - parentForcedColors: ColorsLookup; - forcedColors: ColorsLookup; + labelsColorMapInstance: ReturnType; + + chartLabelsColorMap: Map; + multiple: number; /** * Constructor * @param {*} colors an array of colors - * @param {*} parentForcedColors optional parameter that comes from parent - * (usually CategoricalColorNamespace) and supersede this.forcedColors + * @param {*} forcedColors optional parameter that comes from parent + * (usually CategoricalColorNamespace) */ - constructor(colors: string[], parentForcedColors: ColorsInitLookup = {}) { - super((value: string, sliceId?: number) => this.getColor(value, sliceId)); - + constructor(colors: string[], forcedColors: ColorsInitLookup = {}) { + super((value: string, sliceId?: number, colorScheme?: string) => + this.getColor(value, sliceId, colorScheme), + ); + // holds original color scheme colors this.originColors = colors; + // holds the extended color range (includes analagous colors) this.colors = colors; + // holds the values of this specific slice (label+color) + this.chartLabelsColorMap = new Map(); + // shared color map instance (when context is shared, i.e. dashboard) + this.labelsColorMapInstance = getLabelsColorMap(); + // holds the multiple value for analogous colors range + this.multiple = 0; + this.scale = scaleOrdinal<{ toString(): string }, string>(); this.scale.range(colors); // reserve fixed colors in parent map based on their index in the scale - Object.entries(parentForcedColors).forEach(([key, value]) => { + Object.entries(forcedColors).forEach(([key, value]) => { if (typeof value === 'number') { // eslint-disable-next-line no-param-reassign - parentForcedColors[key] = colors[value % colors.length]; + forcedColors[key] = colors[value % colors.length]; } }); - // all indexes have been replaced by a fixed color - this.parentForcedColors = parentForcedColors as ColorsLookup; - this.forcedColors = {}; - this.multiple = 0; + // forced colors from parent (usually CategoricalColorNamespace) + // currently used in dashboards to set custom label colors + this.forcedColors = forcedColors as ColorsLookup; } - removeSharedLabelColorFromRange( - sharedColorMap: Map, - cleanedValue: string, - ) { - // make sure we don't overwrite the origin colors - const updatedRange = new Set(this.originColors); - // remove the color option from shared color - sharedColorMap.forEach((value: string, key: string) => { - if (key !== cleanedValue) { - updatedRange.delete(value); - } - }); - // remove the color option from forced colors - Object.entries(this.parentForcedColors).forEach(([key, value]) => { - if (key !== cleanedValue) { - updatedRange.delete(value); - } - }); - this.range(updatedRange.size > 0 ? [...updatedRange] : this.originColors); + /** + * Increment the color range with analogous colors + */ + incrementColorRange() { + const multiple = Math.floor( + this.domain().length / this.originColors.length, + ); + // the domain has grown larger than the original range + // increments the range with analogous colors + if (multiple > this.multiple) { + this.multiple = multiple; + const newRange = getAnalogousColors(this.originColors, multiple); + const extendedColors = this.originColors.concat(newRange); + + this.range(extendedColors); + this.colors = extendedColors; + } } - getColor(value?: string, sliceId?: number) { + /** + * Get the color for a given value + * + * @param value the value of a label to get the color for + * @param sliceId the ID of the current chart + * @param colorScheme the original color scheme of the chart + * @returns the color or the next available color + */ + getColor(value?: string, sliceId?: number, colorScheme?: string): string { const cleanedValue = stringifyAndTrim(value); - const sharedLabelColor = getSharedLabelColor(); - const sharedColorMap = sharedLabelColor.getColorMap(); - const sharedColor = sharedColorMap.get(cleanedValue); + // priority: forced color (i.e. custom label colors) > shared color > scale color + const forcedColor = this.forcedColors?.[cleanedValue]; + const isExistingLabel = this.chartLabelsColorMap.has(cleanedValue); + let color = forcedColor || this.scale(cleanedValue); - // priority: parentForcedColors > forcedColors > labelColors - let color = - this.parentForcedColors?.[cleanedValue] || - this.forcedColors?.[cleanedValue] || - sharedColor; + // a forced color will always be used independently of the usage count + if (!forcedColor && !isExistingLabel) { + if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) { + this.incrementColorRange(); + } + if ( + // feature flag to be deprecated (will become standard behaviour) + isFeatureEnabled(FeatureFlag.AvoidColorsCollision) && + this.isColorUsed(color) + ) { + // fallback to least used color + color = this.getNextAvailableColor(color); + } + } - if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) { - const multiple = Math.floor( - this.domain().length / this.originColors.length, + // keep track of values in this slice + this.chartLabelsColorMap.set(cleanedValue, color); + + // store the value+color in the LabelsColorMapSingleton + if (sliceId) { + this.labelsColorMapInstance.addSlice( + cleanedValue, + color, + sliceId, + colorScheme, ); - if (multiple > this.multiple) { - this.multiple = multiple; - const newRange = getAnalogousColors(this.originColors, multiple); - this.range(this.originColors.concat(newRange)); - } } - const newColor = this.scale(cleanedValue); - if (!color) { - color = newColor; - if (isFeatureEnabled(FeatureFlag.AvoidColorsCollision)) { - this.removeSharedLabelColorFromRange(sharedColorMap, cleanedValue); - color = this.scale(cleanedValue); - } - } - - sharedLabelColor.addSlice(cleanedValue, color, sliceId); - return color; } /** - * Enforce specific color for given value + * Verify if a color is used in this slice + * + * @param color + * @returns true if the color is used in this slice + */ + isColorUsed(color: string): boolean { + return this.getColorUsageCount(color) > 0; + } + + /** + * Get the count of the color usage in this slice + * + * @param sliceId the ID of the current slice + * @param color the color to check + * @returns the count of the color usage in this slice + */ + getColorUsageCount(currentColor: string): number { + let count = 0; + this.chartLabelsColorMap.forEach(color => { + if (color === currentColor) { + count += 1; + } + }); + return count; + } + + /** + * Lower chances of color collision by returning the least used color + * Checks across colors of current slice within LabelsColorMapSingleton + * + * @param currentColor the current color + * @returns the least used color that is not the excluded color + */ + getNextAvailableColor(currentColor: string) { + const colorUsageArray = this.colors.map(color => ({ + color, + count: this.getColorUsageCount(color), + })); + const currentColorCount = this.getColorUsageCount(currentColor); + const otherColors = colorUsageArray.filter( + colorEntry => colorEntry.color !== currentColor, + ); + // all other colors are used as much or more than currentColor + const hasNoneAvailable = otherColors.every( + colorEntry => colorEntry.count >= currentColorCount, + ); + + // fallback to currentColor color + if (!otherColors.length || hasNoneAvailable) { + return currentColor; + } + + // Finding the least used color + const leastUsedColor = otherColors.reduce((min, entry) => + entry.count < min.count ? entry : min, + ).color; + + return leastUsedColor; + } + + /** + * Enforce specific color for a given value at the scale level + * Overrides any existing color and forced color for the given value + * * @param {*} value value * @param {*} forcedColor forcedColor + * @returns {CategoricalColorScale} */ setColor(value: string, forcedColor: string) { this.forcedColors[stringifyAndTrim(value)] = forcedColor; @@ -142,6 +223,7 @@ class CategoricalColorScale extends ExtensibleFunction { /** * Get a mapping of data values to colors + * * @returns an object where the key is the data value and the value is the hex color code */ getColorMap() { @@ -153,22 +235,23 @@ class CategoricalColorScale extends ExtensibleFunction { return { ...colorMap, ...this.forcedColors, - ...this.parentForcedColors, }; } /** - * Returns an exact copy of this scale. Changes to this scale will not affect the returned scale, and vice versa. + * Return an exact copy of this scale. + * Changes to this scale will not affect the returned scale and vice versa. + * + * @returns {CategoricalColorScale} A copy of this scale. */ copy() { const copy = new CategoricalColorScale( this.scale.range(), - this.parentForcedColors, + this.forcedColors, ); copy.forcedColors = { ...this.forcedColors }; copy.domain(this.domain()); copy.unknown(this.unknown()); - return copy; } diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts similarity index 52% rename from superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts rename to superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts index 4837d0f362..59d3f8cc5d 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts @@ -17,36 +17,37 @@ * under the License. */ -import { CategoricalColorNamespace } from '.'; import { makeSingleton } from '../utils'; -export enum SharedLabelColorSource { +export enum LabelsColorMapSource { Dashboard, Explore, } -export class SharedLabelColor { - sliceLabelMap: Map; + +export class LabelsColorMap { + chartsLabelsMap: Map; colorMap: Map; - source: SharedLabelColorSource; + source: LabelsColorMapSource; constructor() { - // { sliceId1: [label1, label2, ...], sliceId2: [label1, label2, ...] } - this.sliceLabelMap = new Map(); + // holds labels and original color schemes for each chart in context + this.chartsLabelsMap = new Map(); this.colorMap = new Map(); - this.source = SharedLabelColorSource.Dashboard; + this.source = LabelsColorMapSource.Dashboard; } - updateColorMap(colorNamespace?: string, colorScheme?: string) { - const categoricalNamespace = - CategoricalColorNamespace.getNamespace(colorNamespace); + updateColorMap(categoricalNamespace: any, colorScheme?: string) { const newColorMap = new Map(); this.colorMap.clear(); - this.sliceLabelMap.forEach(labels => { - const colorScale = categoricalNamespace.getScale(colorScheme); + this.chartsLabelsMap.forEach((chartConfig, sliceId) => { + const { labels, scheme: originalChartColorScheme } = chartConfig; + const currentColorScheme = colorScheme || originalChartColorScheme; + const colorScale = categoricalNamespace.getScale(currentColorScheme); + labels.forEach(label => { - const newColor = colorScale(label); + const newColor = colorScale.getColor(label, sliceId); newColorMap.set(label, newColor); }); }); @@ -57,25 +58,37 @@ export class SharedLabelColor { return this.colorMap; } - addSlice(label: string, color: string, sliceId?: number) { - if ( - this.source !== SharedLabelColorSource.Dashboard || - sliceId === undefined - ) - return; - const labels = this.sliceLabelMap.get(sliceId) || []; + addSlice( + label: string, + color: string, + sliceId: number, + colorScheme?: string, + ) { + if (this.source !== LabelsColorMapSource.Dashboard) return; + + const chartConfig = this.chartsLabelsMap.get(sliceId) || { + labels: [], + scheme: '', + }; + const { labels } = chartConfig; if (!labels.includes(label)) { labels.push(label); - this.sliceLabelMap.set(sliceId, labels); + this.chartsLabelsMap.set(sliceId, { + labels, + scheme: colorScheme, + }); } this.colorMap.set(label, color); } removeSlice(sliceId: number) { - if (this.source !== SharedLabelColorSource.Dashboard) return; - this.sliceLabelMap.delete(sliceId); + if (this.source !== LabelsColorMapSource.Dashboard) return; + + this.chartsLabelsMap.delete(sliceId); const newColorMap = new Map(); - this.sliceLabelMap.forEach(labels => { + + this.chartsLabelsMap.forEach(chartConfig => { + const { labels } = chartConfig; labels.forEach(label => { newColorMap.set(label, this.colorMap.get(label)); }); @@ -83,19 +96,12 @@ export class SharedLabelColor { this.colorMap = newColorMap; } - reset() { - const copyColorMap = new Map(this.colorMap); - copyColorMap.forEach((_, label) => { - this.colorMap.set(label, ''); - }); - } - clear() { - this.sliceLabelMap.clear(); + this.chartsLabelsMap.clear(); this.colorMap.clear(); } } -const getInstance = makeSingleton(SharedLabelColor); +const getInstance = makeSingleton(LabelsColorMap); export default getInstance; 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 cb7e569b47..5e4d0ea7aa 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/index.ts @@ -34,9 +34,9 @@ export * from './colorSchemes'; export * from './utils'; export * from './types'; export { - default as getSharedLabelColor, - SharedLabelColor, - SharedLabelColorSource, -} from './SharedLabelColorSingleton'; + default as getLabelsColorMap, + LabelsColorMap, + LabelsColorMapSource, +} from './LabelsColorMapSingleton'; export const BRAND_COLOR = '#00A699'; diff --git a/superset-frontend/packages/superset-ui-core/src/color/utils.ts b/superset-frontend/packages/superset-ui-core/src/color/utils.ts index 55284f16b4..c0f33f9f8c 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/utils.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/utils.ts @@ -55,11 +55,12 @@ export function getContrastingColor(color: string, thresholds = 186) { export function getAnalogousColors(colors: string[], results: number) { const generatedColors: string[] = []; - // This is to solve the problem that the first three values generated by tinycolor.analogous - // may have the same or very close colors. const ext = 3; + const analogousColors = colors.map(color => { + // returns an array of tinycolor instances const result = tinycolor(color).analogous(results + ext); + // remove the first three colors to avoid the same or very close colors return result.slice(ext); }); diff --git a/superset-frontend/packages/superset-ui-core/src/hooks/useComponentDidUpdate/useComponentDidUpdate.ts b/superset-frontend/packages/superset-ui-core/src/hooks/useComponentDidUpdate/useComponentDidUpdate.ts index aa84568e29..f382cd37c8 100644 --- a/superset-frontend/packages/superset-ui-core/src/hooks/useComponentDidUpdate/useComponentDidUpdate.ts +++ b/superset-frontend/packages/superset-ui-core/src/hooks/useComponentDidUpdate/useComponentDidUpdate.ts @@ -17,9 +17,12 @@ * under the License. */ -import { EffectCallback, useEffect, useRef } from 'react'; +import { DependencyList, EffectCallback, useEffect, useRef } from 'react'; -export const useComponentDidUpdate = (effect: EffectCallback) => { +export const useComponentDidUpdate = ( + effect: EffectCallback, + deps?: DependencyList, +) => { const isMountedRef = useRef(false); useEffect(() => { if (isMountedRef.current) { @@ -27,5 +30,6 @@ export const useComponentDidUpdate = (effect: EffectCallback) => { } else { isMountedRef.current = true; } - }, [effect]); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [...(deps || [effect])]); }; diff --git a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts index d5e1ef8f0b..69fb38eea3 100644 --- a/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/color/CategoricalColorNameSpace.test.ts @@ -98,7 +98,7 @@ describe('CategoricalColorNamespace', () => { namespace.setColor('dog', 'black'); const scale = namespace.getScale('testColors'); scale.setColor('dog', 'pink'); - expect(scale.getColor('dog')).toBe('black'); + expect(scale.getColor('dog')).toBe('pink'); expect(scale.getColor('boy')).not.toBe('black'); }); it('does not affect scales in other namespaces', () => { 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 48e43d8351..97d756cb04 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 @@ -18,50 +18,76 @@ */ import { ScaleOrdinal } from 'd3-scale'; -import { - CategoricalColorScale, - FeatureFlag, - getSharedLabelColor, -} from '@superset-ui/core'; +import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core'; describe('CategoricalColorScale', () => { + beforeEach(() => { + window.featureFlags = {}; + }); + it('exists', () => { expect(CategoricalColorScale !== undefined).toBe(true); }); - describe('new CategoricalColorScale(colors, parentForcedColors)', () => { - it('can create new scale when parentForcedColors is not given', () => { + describe('new CategoricalColorScale(colors, forcedColors)', () => { + it('can create new scale when forcedColors is not given', () => { const scale = new CategoricalColorScale(['blue', 'red', 'green']); expect(scale).toBeInstanceOf(CategoricalColorScale); }); - it('can create new scale when parentForcedColors is given', () => { - const parentForcedColors = {}; + it('can create new scale when forcedColors is given', () => { + const forcedColors = {}; const scale = new CategoricalColorScale( ['blue', 'red', 'green'], - parentForcedColors, + forcedColors, ); expect(scale).toBeInstanceOf(CategoricalColorScale); - expect(scale.parentForcedColors).toBe(parentForcedColors); + expect(scale.forcedColors).toBe(forcedColors); }); it('can refer to colors based on their index', () => { - const parentForcedColors = { pig: 1, horse: 5 }; + const forcedColors = { pig: 1, horse: 5 }; const scale = new CategoricalColorScale( ['blue', 'red', 'green'], - parentForcedColors, + forcedColors, ); expect(scale.getColor('pig')).toEqual('red'); - expect(parentForcedColors.pig).toEqual('red'); + expect(forcedColors.pig).toEqual('red'); // can loop around the scale expect(scale.getColor('horse')).toEqual('green'); - expect(parentForcedColors.horse).toEqual('green'); + expect(forcedColors.horse).toEqual('green'); }); }); - describe('.getColor(value)', () => { + describe('.getColor(value, sliceId)', () => { + let scale: CategoricalColorScale; + let addSliceSpy: jest.SpyInstance< + void, + [label: string, color: string, sliceId: number, colorScheme?: string] + >; + let getNextAvailableColorSpy: jest.SpyInstance< + string, + [currentColor: string] + >; + + beforeEach(() => { + scale = new CategoricalColorScale(['blue', 'red', 'green']); + // Spy on the addSlice method of labelsColorMapInstance + addSliceSpy = jest.spyOn(scale.labelsColorMapInstance, 'addSlice'); + getNextAvailableColorSpy = jest + .spyOn(scale, 'getNextAvailableColor') + .mockImplementation(color => color); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + it('returns same color for same value', () => { - const scale = new CategoricalColorScale(['blue', 'red', 'green']); + const scale = new CategoricalColorScale(['blue', 'red', 'green'], { + pig: 'red', + horse: 'green', + }); const c1 = scale.getColor('pig'); const c2 = scale.getColor('horse'); const c3 = scale.getColor('pig'); @@ -82,9 +108,6 @@ describe('CategoricalColorScale', () => { expect(c3).not.toBe(c1); }); it('recycles colors when number of items exceed available colors', () => { - window.featureFlags = { - [FeatureFlag.UseAnalagousColors]: false, - }; const colorSet: { [key: string]: number } = {}; const scale = new CategoricalColorScale(['blue', 'red', 'green']); const colors = [ @@ -118,43 +141,70 @@ describe('CategoricalColorScale', () => { scale.getColor('cow'); scale.getColor('donkey'); scale.getColor('goat'); - expect(scale.range()).toHaveLength(6); + expect(scale.range()).toHaveLength(9); }); + it('adds the color and value to chartLabelsColorMap and calls addSlice', () => { + const value = 'testValue'; + const sliceId = 123; + const colorScheme = 'preset'; - it('should remove shared color from range if avoid colors collision enabled', () => { + expect(scale.chartLabelsColorMap.has(value)).toBe(false); + + scale.getColor(value, sliceId, colorScheme); + + expect(scale.chartLabelsColorMap.has(value)).toBe(true); + expect(scale.chartLabelsColorMap.get(value)).toBeDefined(); + + expect(addSliceSpy).toHaveBeenCalledWith( + value, + expect.any(String), + sliceId, + colorScheme, + ); + + const expectedColor = scale.chartLabelsColorMap.get(value); + const returnedColor = scale.getColor(value, sliceId); + expect(returnedColor).toBe(expectedColor); + }); + it('conditionally calls getNextAvailableColor', () => { window.featureFlags = { [FeatureFlag.AvoidColorsCollision]: true, }; - const scale = new CategoricalColorScale(['blue', 'red', 'green']); - const color1 = scale.getColor('a', 1); - expect(scale.range()).toHaveLength(3); - const color2 = scale.getColor('a', 2); - expect(color1).toBe(color2); - scale.getColor('b', 2); - expect(scale.range()).toHaveLength(2); - scale.getColor('c', 2); - expect(scale.range()).toHaveLength(1); + + scale.getColor('testValue1'); + scale.getColor('testValue2'); + scale.getColor('testValue1'); + scale.getColor('testValue3'); + scale.getColor('testValue4'); + + expect(getNextAvailableColorSpy).toHaveBeenCalledWith('blue'); + + getNextAvailableColorSpy.mockClear(); + + window.featureFlags = { + [FeatureFlag.AvoidColorsCollision]: false, + }; + + scale.getColor('testValue3'); + + expect(getNextAvailableColorSpy).not.toHaveBeenCalled(); }); - window.featureFlags = { - [FeatureFlag.AvoidColorsCollision]: false, - }; }); + describe('.setColor(value, forcedColor)', () => { it('overrides default color', () => { const scale = new CategoricalColorScale(['blue', 'red', 'green']); scale.setColor('pig', 'pink'); expect(scale.getColor('pig')).toBe('pink'); }); - it('does not override parentForcedColors', () => { + it('does override forcedColors', () => { const scale1 = new CategoricalColorScale(['blue', 'red', 'green']); scale1.setColor('pig', 'black'); - const scale2 = new CategoricalColorScale( - ['blue', 'red', 'green'], - scale1.forcedColors, - ); + + const scale2 = new CategoricalColorScale(['blue', 'red', 'green']); scale2.setColor('pig', 'pink'); + expect(scale2.getColor('pig')).toBe('pink'); expect(scale1.getColor('pig')).toBe('black'); - expect(scale2.getColor('pig')).toBe('black'); }); it('returns the scale', () => { const scale = new CategoricalColorScale(['blue', 'red', 'green']); @@ -163,7 +213,7 @@ describe('CategoricalColorScale', () => { }); }); describe('.getColorMap()', () => { - it('returns correct mapping and parentForcedColors and forcedColors are specified', () => { + it('returns correct mapping using least used color', () => { const scale1 = new CategoricalColorScale(['blue', 'red', 'green']); scale1.setColor('cow', 'black'); const scale2 = new CategoricalColorScale( @@ -177,7 +227,7 @@ describe('CategoricalColorScale', () => { expect(scale2.getColorMap()).toEqual({ cow: 'black', pig: 'pink', - horse: 'green', + horse: 'blue', // least used color }); }); }); @@ -230,10 +280,114 @@ describe('CategoricalColorScale', () => { }); describe('a CategoricalColorScale instance is also a color function itself', () => { - it('scale(value) returns color similar to calling scale.getColor(value)', () => { + it('scale(value) returns same color for same value', () => { const scale = new CategoricalColorScale(['blue', 'red', 'green']); - expect(scale.getColor('pig')).toBe(scale('pig')); - expect(scale.getColor('cat')).toBe(scale('cat')); + expect(scale.getColor('pig')).toBe('blue'); + expect(scale('pig')).toBe('blue'); + expect(scale.getColor('cat')).toBe('red'); + expect(scale('cat')).toBe('red'); + }); + }); + + describe('.getNextAvailableColor(currentColor)', () => { + it('returns the current color if it is the least used or equally used among colors', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + scale.getColor('cat'); + scale.getColor('dog'); + + // Since 'green' hasn't been used, it's considered the least used. + expect(scale.getNextAvailableColor('blue')).toBe('green'); + }); + + it('handles cases where all colors are equally used and returns the current color', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + scale.getColor('cat'); // blue + scale.getColor('dog'); // red + scale.getColor('fish'); // green + // All colors used once, so the function should return the current color + expect(scale.getNextAvailableColor('red')).toBe('red'); + }); + + it('returns the least used color accurately even when some colors are used more frequently', () => { + const scale = new CategoricalColorScale([ + 'blue', + 'red', + 'green', + 'yellow', + ]); + scale.getColor('cat'); // blue + scale.getColor('dog'); // red + scale.getColor('frog'); // green + scale.getColor('fish'); // yellow + scale.getColor('goat'); // blue + scale.getColor('horse'); // red + scale.getColor('pony'); // green + + // Yellow is the least used color, so it should be returned. + expect(scale.getNextAvailableColor('blue')).toBe('yellow'); + }); + }); + + describe('.isColorUsed(color)', () => { + it('returns true if the color is already used, false otherwise', () => { + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + // Initially, no color is used + expect(scale.isColorUsed('blue')).toBe(false); + expect(scale.isColorUsed('red')).toBe(false); + expect(scale.isColorUsed('green')).toBe(false); + + scale.getColor('item1'); + + // Now, 'blue' is used, but 'red' and 'green' are not + expect(scale.isColorUsed('blue')).toBe(true); + expect(scale.isColorUsed('red')).toBe(false); + expect(scale.isColorUsed('green')).toBe(false); + + // Simulate using the 'red' color + scale.getColor('item2'); // Assigns 'red' to 'item2' + + // Now, 'blue' and 'red' are used + expect(scale.isColorUsed('blue')).toBe(true); + expect(scale.isColorUsed('red')).toBe(true); + expect(scale.isColorUsed('green')).toBe(false); + }); + }); + + describe('.getColorUsageCount(color)', () => { + it('accurately counts the occurrences of a specific color', () => { + const scale = new CategoricalColorScale([ + 'blue', + 'red', + 'green', + 'yellow', + ]); + // No colors are used initially + expect(scale.getColorUsageCount('blue')).toBe(0); + expect(scale.getColorUsageCount('red')).toBe(0); + expect(scale.getColorUsageCount('green')).toBe(0); + expect(scale.getColorUsageCount('yellow')).toBe(0); + + // Simulate using colors + scale.getColor('item1'); + scale.getColor('item2'); + scale.getColor('item1'); + + // Check the counts after using the colors + expect(scale.getColorUsageCount('blue')).toBe(1); + expect(scale.getColorUsageCount('red')).toBe(1); + expect(scale.getColorUsageCount('green')).toBe(0); + expect(scale.getColorUsageCount('yellow')).toBe(0); + + // Simulate using colors more + scale.getColor('item3'); + scale.getColor('item4'); + scale.getColor('item3'); + + // Final counts + expect(scale.getColorUsageCount('blue')).toBe(1); + expect(scale.getColorUsageCount('red')).toBe(1); + expect(scale.getColorUsageCount('green')).toBe(1); + expect(scale.getColorUsageCount('yellow')).toBe(1); }); }); @@ -244,50 +398,4 @@ describe('CategoricalColorScale', () => { expect(scale('pig')).toBe('blue'); }); }); - - describe('.removeSharedLabelColorFromRange(colorMap, cleanedValue)', () => { - it('should remove shared color from range', () => { - const scale = new CategoricalColorScale(['blue', 'green', 'red']); - expect(scale.range()).toEqual(['blue', 'green', 'red']); - - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.clear(); - const colorMap = sharedLabelColor.getColorMap(); - sharedLabelColor.addSlice('cow', 'blue', 1); - scale.removeSharedLabelColorFromRange(colorMap, 'pig'); - expect(scale.range()).toEqual(['green', 'red']); - scale.removeSharedLabelColorFromRange(colorMap, 'cow'); - expect(scale.range()).toEqual(['blue', 'green', 'red']); - sharedLabelColor.clear(); - }); - - it('recycles colors when all colors are in sharedLabelColor', () => { - const scale = new CategoricalColorScale(['blue', 'green', 'red']); - expect(scale.range()).toEqual(['blue', 'green', 'red']); - const sharedLabelColor = getSharedLabelColor(); - const colorMap = sharedLabelColor.getColorMap(); - sharedLabelColor.addSlice('cow', 'blue', 1); - sharedLabelColor.addSlice('pig', 'red', 1); - sharedLabelColor.addSlice('horse', 'green', 1); - scale.removeSharedLabelColorFromRange(colorMap, 'goat'); - expect(scale.range()).toEqual(['blue', 'green', 'red']); - sharedLabelColor.clear(); - }); - - it('should remove parentForcedColors from range', () => { - const parentForcedColors = { house: 'blue', cow: 'red' }; - const scale = new CategoricalColorScale( - ['blue', 'red', 'green'], - parentForcedColors, - ); - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.clear(); - const colorMap = sharedLabelColor.getColorMap(); - scale.removeSharedLabelColorFromRange(colorMap, 'pig'); - expect(scale.range()).toEqual(['green']); - scale.removeSharedLabelColorFromRange(colorMap, 'cow'); - expect(scale.range()).toEqual(['red', 'green']); - sharedLabelColor.clear(); - }); - }); }); diff --git a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts new file mode 100644 index 0000000000..b93a416e7f --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts @@ -0,0 +1,234 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + CategoricalColorNamespace, + CategoricalScheme, + FeatureFlag, + getCategoricalSchemeRegistry, + getLabelsColorMap, + LabelsColorMapSource, + LabelsColorMap, +} from '@superset-ui/core'; + +const actual = jest.requireActual('../../src/color/utils'); +const getAnalogousColorsSpy = jest + .spyOn(actual, 'getAnalogousColors') + .mockImplementation(() => ['red', 'green', 'blue']); + +describe('LabelsColorMap', () => { + beforeAll(() => { + getCategoricalSchemeRegistry() + .registerValue( + 'testColors', + new CategoricalScheme({ + id: 'testColors', + colors: ['red', 'green', 'blue'], + }), + ) + .registerValue( + 'testColors2', + new CategoricalScheme({ + id: 'testColors2', + colors: ['yellow', 'green', 'blue'], + }), + ); + }); + + beforeEach(() => { + getLabelsColorMap().source = LabelsColorMapSource.Dashboard; + getLabelsColorMap().clear(); + }); + + it('has default value out-of-the-box', () => { + expect(getLabelsColorMap()).toBeInstanceOf(LabelsColorMap); + }); + + describe('.addSlice(value, color, sliceId)', () => { + it('should add to sliceLabelColorMap when first adding label', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1, 'preset'); + expect(labelsColorMap.chartsLabelsMap.has(1)).toEqual(true); + const chartConfig = labelsColorMap.chartsLabelsMap.get(1); + expect(chartConfig?.labels?.includes('a')).toEqual(true); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red' }); + }); + + it('should add to sliceLabelColorMap when slice exist', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 1); + const chartConfig = labelsColorMap.chartsLabelsMap.get(1); + expect(chartConfig?.labels?.includes('b')).toEqual(true); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red', b: 'blue' }); + }); + + it('should use last color if adding label repeatedly', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('b', 'blue', 1); + labelsColorMap.addSlice('b', 'green', 1); + const chartConfig = labelsColorMap.chartsLabelsMap.get(1); + expect(chartConfig?.labels?.includes('b')).toEqual(true); + expect(chartConfig?.labels?.length).toEqual(1); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ b: 'green' }); + }); + + it('should do nothing when source is not dashboard', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.source = LabelsColorMapSource.Explore; + labelsColorMap.addSlice('a', 'red', 1); + expect(Object.fromEntries(labelsColorMap.chartsLabelsMap)).toEqual({}); + }); + }); + + describe('.remove(sliceId)', () => { + it('should remove sliceId', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.removeSlice(1); + expect(labelsColorMap.chartsLabelsMap.has(1)).toEqual(false); + }); + + it('should update colorMap', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 2); + labelsColorMap.removeSlice(1); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ b: 'blue' }); + }); + + it('should do nothing when source is not dashboard', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.source = LabelsColorMapSource.Explore; + labelsColorMap.removeSlice(1); + expect(labelsColorMap.chartsLabelsMap.has(1)).toEqual(true); + }); + }); + + describe('.updateColorMap(namespace, scheme)', () => { + let categoricalNamespace: any; + let mockedNamespace: any; + let labelsColorMap: any; + + beforeEach(() => { + labelsColorMap = getLabelsColorMap(); + categoricalNamespace = CategoricalColorNamespace.getNamespace(undefined); + mockedNamespace = { + getScale: jest.fn().mockReturnValue({ + getColor: jest.fn(() => 'mockColor'), + }), + }; + }); + + it('should use provided color scheme', () => { + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.updateColorMap(mockedNamespace, 'testColors2'); + expect(mockedNamespace.getScale).toHaveBeenCalledWith('testColors2'); + }); + + it('should fallback to original chart color scheme if no color scheme is provided', () => { + labelsColorMap.addSlice('a', 'red', 1, 'originalScheme'); + labelsColorMap.updateColorMap(mockedNamespace); + expect(mockedNamespace.getScale).toHaveBeenCalledWith('originalScheme'); + }); + + it('should fallback to undefined if no color scheme is provided', () => { + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 2); + labelsColorMap.updateColorMap(mockedNamespace); + expect(mockedNamespace.getScale).toHaveBeenCalledWith(undefined); + }); + + it('should update color map', () => { + // override color with forcedItems + categoricalNamespace.setColor('b', 'green'); + // testColors2: 'yellow', 'green', 'blue' + // first-time label, gets color, yellow + labelsColorMap.addSlice('a', 'red', 1); + // overridden, gets green + labelsColorMap.addSlice('b', 'pink', 1); + // overridden, gets green + labelsColorMap.addSlice('b', 'green', 2); + // first-time slice label, gets color, yellow + labelsColorMap.addSlice('c', 'blue', 2); + labelsColorMap.updateColorMap(categoricalNamespace, 'testColors2'); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ + a: 'yellow', + b: 'green', + c: 'yellow', + }); + }); + + it('should use recycle colors', () => { + window.featureFlags = { + [FeatureFlag.UseAnalagousColors]: false, + }; + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 2); + labelsColorMap.addSlice('c', 'green', 3); + labelsColorMap.addSlice('d', 'red', 4); + labelsColorMap.updateColorMap(categoricalNamespace, 'testColors'); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).not.toEqual({}); + expect(getAnalogousColorsSpy).not.toBeCalled(); + }); + + it('should use analagous colors', () => { + window.featureFlags = { + [FeatureFlag.UseAnalagousColors]: true, + }; + + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 1); + labelsColorMap.addSlice('c', 'green', 1); + labelsColorMap.addSlice('d', 'red', 1); + labelsColorMap.updateColorMap(categoricalNamespace, 'testColors'); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).not.toEqual({}); + expect(getAnalogousColorsSpy).toBeCalled(); + }); + }); + + describe('.getColorMap()', () => { + it('should get color map', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 2); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({ a: 'red', b: 'blue' }); + }); + }); + + describe('.reset()', () => { + it('should reset color map', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.addSlice('b', 'blue', 2); + labelsColorMap.clear(); + const colorMap = labelsColorMap.getColorMap(); + expect(Object.fromEntries(colorMap)).toEqual({}); + }); + }); +}); 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 deleted file mode 100644 index ca3f89a523..0000000000 --- a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts +++ /dev/null @@ -1,201 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { - CategoricalScheme, - FeatureFlag, - getCategoricalSchemeRegistry, - getSharedLabelColor, - SharedLabelColor, - SharedLabelColorSource, -} from '@superset-ui/core'; - -const actual = jest.requireActual('../../src/color/utils'); -const getAnalogousColorsSpy = jest - .spyOn(actual, 'getAnalogousColors') - .mockImplementation(() => ['red', 'green', 'blue']); - -describe('SharedLabelColor', () => { - beforeAll(() => { - getCategoricalSchemeRegistry() - .registerValue( - 'testColors', - new CategoricalScheme({ - id: 'testColors', - colors: ['red', 'green', 'blue'], - }), - ) - .registerValue( - 'testColors2', - new CategoricalScheme({ - id: 'testColors2', - colors: ['yellow', 'green', 'blue'], - }), - ); - }); - - beforeEach(() => { - getSharedLabelColor().source = SharedLabelColorSource.Dashboard; - getSharedLabelColor().clear(); - }); - - it('has default value out-of-the-box', () => { - expect(getSharedLabelColor()).toBeInstanceOf(SharedLabelColor); - }); - - describe('.addSlice(value, color, sliceId)', () => { - it('should add to sliceLabelColorMap when first adding label', () => { - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - 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(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); - }); - }); - - describe('.remove(sliceId)', () => { - it('should remove sliceId', () => { - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.removeSlice(1); - 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', () => { - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - sharedLabelColor.source = SharedLabelColorSource.Explore; - sharedLabelColor.removeSlice(1); - expect(sharedLabelColor.sliceLabelMap.has(1)).toEqual(true); - }); - }); - - describe('.updateColorMap(namespace, scheme)', () => { - 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: 'yellow', - c: 'green', - }); - }); - - it('should use recycle colors', () => { - window.featureFlags = { - [FeatureFlag.UseAnalagousColors]: false, - }; - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - 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(getAnalogousColorsSpy).not.toBeCalled(); - }); - - it('should use analagous colors', () => { - window.featureFlags = { - [FeatureFlag.UseAnalagousColors]: true, - }; - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.addSlice('a', 'red', 1); - 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(getAnalogousColorsSpy).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/plugins/legacy-plugin-chart-chord/src/Chord.js b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js index d0aed798de..2daed05f47 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js +++ b/superset-frontend/plugins/legacy-plugin-chart-chord/src/Chord.js @@ -93,7 +93,7 @@ function Chord(element, props) { .append('path') .attr('id', (d, i) => `group${i}`) .attr('d', arc) - .style('fill', (d, i) => colorFn(nodes[i], sliceId)); + .style('fill', (d, i) => colorFn(nodes[i], sliceId, colorScheme)); // Add a text label. const groupText = group.append('text').attr('x', 6).attr('dy', 15); @@ -121,7 +121,7 @@ function Chord(element, props) { .on('mouseover', d => { chord.classed('fade', p => p !== d); }) - .style('fill', d => colorFn(nodes[d.source.index], sliceId)) + .style('fill', d => colorFn(nodes[d.source.index], sliceId, colorScheme)) .attr('d', path); // Add an elaborate mouseover title for each chord. diff --git a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx index 535f3c3b63..0af5f8bf77 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx +++ b/superset-frontend/plugins/legacy-plugin-chart-histogram/src/Histogram.jsx @@ -78,7 +78,7 @@ class CustomHistogram extends PureComponent { const keys = data.map(d => d.key); const colorScale = scaleOrdinal({ domain: keys, - range: keys.map(x => colorFn(x, sliceId)), + range: keys.map(x => colorFn(x, sliceId, colorScheme)), }); return ( diff --git a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js index 2247061668..e0ebd559a9 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js +++ b/superset-frontend/plugins/legacy-plugin-chart-partition/src/Partition.js @@ -384,7 +384,7 @@ function Icicle(element, props) { // Apply color scheme g.selectAll('rect').style('fill', d => { - d.color = colorFn(d.name, sliceId); + d.color = colorFn(d.name, sliceId, colorScheme); return d.color; }); diff --git a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js index 2dfa2ffdd7..e54fc0b6c5 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js +++ b/superset-frontend/plugins/legacy-plugin-chart-rose/src/Rose.js @@ -120,10 +120,16 @@ function Rose(element, props) { .map(v => ({ key: v.name, value: v.value, - color: colorFn(v.name, sliceId), + color: colorFn(v.name, sliceId, colorScheme), highlight: v.id === d.arcId, })) - : [{ key: d.name, value: d.val, color: colorFn(d.name, sliceId) }]; + : [ + { + key: d.name, + value: d.val, + color: colorFn(d.name, sliceId, colorScheme), + }, + ]; return { key: 'Date', @@ -132,7 +138,7 @@ function Rose(element, props) { }; } - legend.width(width).color(d => colorFn(d.key, sliceId)); + legend.width(width).color(d => colorFn(d.key, sliceId, colorScheme)); legendWrap.datum(legendData(datum)).call(legend); tooltip.headerFormatter(timeFormat).valueFormatter(format); @@ -379,7 +385,7 @@ function Rose(element, props) { const arcs = ae .append('path') .attr('class', 'arc') - .attr('fill', d => colorFn(d.name, sliceId)) + .attr('fill', d => colorFn(d.name, sliceId, colorScheme)) .attr('d', arc); function mousemove() { diff --git a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js index d8c8f61e44..0639edad45 100644 --- a/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js +++ b/superset-frontend/plugins/legacy-plugin-chart-sankey/src/Sankey.js @@ -219,7 +219,7 @@ function Sankey(element, props) { .attr('width', sankey.nodeWidth()) .style('fill', d => { const name = d.name || 'N/A'; - d.color = colorFn(name, sliceId); + d.color = colorFn(name, sliceId, colorScheme); return d.color; }) diff --git a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx index 2b81b8a8d2..7dff2af221 100644 --- a/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx +++ b/superset-frontend/plugins/legacy-preset-chart-deckgl/src/CategoricalDeckGLContainer.tsx @@ -58,7 +58,10 @@ function getCategories(fd: QueryFormData, data: JsonObject[]) { if (d.cat_color != null && !categories.hasOwnProperty(d.cat_color)) { let color; if (fd.dimension) { - color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); + color = hexToRGB( + colorFn(d.cat_color, fd.sliceId, fd.color_scheme), + c.a * 255, + ); } else { color = fixedColor; } @@ -134,7 +137,10 @@ const CategoricalDeckGLContainer = (props: CategoricalDeckGLContainerProps) => { return data.map(d => { let color; if (fd.dimension) { - color = hexToRGB(colorFn(d.cat_color, fd.sliceId), c.a * 255); + color = hexToRGB( + colorFn(d.cat_color, fd.sliceId, fd.color_scheme), + c.a * 255, + ); return { ...d, color }; } diff --git a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js index a454fe28b4..06455f16d8 100644 --- a/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js +++ b/superset-frontend/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js @@ -658,7 +658,9 @@ function nvd3Vis(element, props) { } else if (vizType !== 'bullet') { const colorFn = getScale(colorScheme); chart.color( - d => d.color || colorFn(cleanColorInput(d[colorKey]), sliceId), + d => + d.color || + colorFn(cleanColorInput(d[colorKey]), sliceId, colorScheme), ); } diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts index 0edeadb7fa..4f5c8f323f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts @@ -108,9 +108,9 @@ export default function transformProps( datum[`${metric}__outliers`], ], itemStyle: { - color: colorFn(groupbyLabel, sliceId), + color: colorFn(groupbyLabel, sliceId, colorScheme), opacity: isFiltered ? OpacityEnum.SemiTransparent : 0.6, - borderColor: colorFn(groupbyLabel, sliceId), + borderColor: colorFn(groupbyLabel, sliceId, colorScheme), }, }; }); @@ -149,7 +149,7 @@ export default function transformProps( }, }, itemStyle: { - color: colorFn(groupbyLabel, sliceId), + color: colorFn(groupbyLabel, sliceId, colorScheme), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts index daca2eb678..a0b0569337 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts @@ -174,7 +174,7 @@ export default function transformProps( value, name, itemStyle: { - color: colorFn(name, sliceId), + color: colorFn(name, sliceId, colorScheme), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts index 868cc0df36..187ce67dcb 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Gauge/transformProps.ts @@ -173,7 +173,7 @@ export default function transformProps( value: data_point[metricLabel] as number, name, itemStyle: { - color: colorFn(index, sliceId), + color: colorFn(index, sliceId, colorScheme), }, title: { offsetCenter: [ @@ -201,7 +201,7 @@ export default function transformProps( item = { ...item, itemStyle: { - color: colorFn(index, sliceId), + color: colorFn(index, sliceId, colorScheme), opacity: OpacityEnum.SemiTransparent, }, detail: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts index e79b5ac874..2cc3aff92f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts @@ -277,7 +277,7 @@ export default function transformProps( type: 'graph', categories: categoryList.map(c => ({ name: c, - itemStyle: { color: colorFn(c, sliceId) }, + itemStyle: { color: colorFn(c, sliceId, colorScheme) }, })), layout, force: { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts index 40d49908e9..1b3898d8fd 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts @@ -222,7 +222,7 @@ export default function transformProps( value, name, itemStyle: { - color: colorFn(name, sliceId), + color: colorFn(name, sliceId, colorScheme), opacity: isFiltered ? OpacityEnum.SemiTransparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts index c8c3c7ed15..dd49a1b872 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Radar/transformProps.ts @@ -165,7 +165,7 @@ export default function transformProps( value: metricLabels.map(metricLabel => datum[metricLabel]), name: joinedName, itemStyle: { - color: colorFn(joinedName, sliceId), + color: colorFn(joinedName, sliceId, colorScheme), opacity: isFiltered ? OpacityEnum.Transparent : OpacityEnum.NonTransparent, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts index 09bdd19154..73feda2b2e 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts @@ -183,7 +183,7 @@ export default function transformProps( colorSaturation: COLOR_SATURATION, itemStyle: { borderColor: BORDER_COLOR, - color: colorFn(name, sliceId), + color: colorFn(name, sliceId, colorScheme), borderWidth: BORDER_WIDTH, gapWidth: GAP_WIDTH, }, @@ -216,7 +216,7 @@ export default function transformProps( colorSaturation: COLOR_SATURATION, itemStyle: { borderColor: BORDER_COLOR, - color: colorFn(`${metricLabel}`, sliceId), + color: colorFn(`${metricLabel}`, sliceId, colorScheme), borderWidth: BORDER_WIDTH, gapWidth: GAP_WIDTH, }, diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx index fa23001283..53bf98fe90 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx @@ -66,6 +66,7 @@ export interface WordCloudProps extends WordCloudVisualProps { height: number; width: number; sliceId: number; + colorScheme: string; } export interface WordCloudState { @@ -221,7 +222,7 @@ class WordCloud extends PureComponent { render() { const { scaleFactor } = this.state; - const { width, height, encoding, sliceId } = this.props; + const { width, height, encoding, sliceId, colorScheme } = this.props; const { words } = this.state; // @ts-ignore @@ -249,7 +250,11 @@ class WordCloud extends PureComponent { fontSize={`${w.size}px`} fontWeight={w.weight} fontFamily={w.font} - fill={colorFn(getValueFromDatum(w) as string, sliceId)} + fill={colorFn( + getValueFromDatum(w) as string, + sliceId, + colorScheme, + )} textAnchor="middle" transform={`translate(${w.x}, ${w.y}) rotate(${w.rotate})`} > diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts index 0b5a36b999..5685edd921 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/legacyPlugin/transformProps.ts @@ -80,5 +80,6 @@ export default function transformProps(chartProps: ChartProps): WordCloudProps { rotation, width, sliceId, + colorScheme, }; } diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts index 4ed6f6405c..59f6258538 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/transformProps.ts @@ -23,7 +23,8 @@ import { WordCloudFormData } from '../types'; export default function transformProps(chartProps: ChartProps): WordCloudProps { const { width, height, formData, queriesData } = chartProps; - const { encoding, rotation, sliceId } = formData as WordCloudFormData; + const { encoding, rotation, sliceId, colorScheme } = + formData as WordCloudFormData; return { data: queriesData[0].data, @@ -32,5 +33,6 @@ export default function transformProps(chartProps: ChartProps): WordCloudProps { rotation, width, sliceId, + colorScheme, }; } diff --git a/superset-frontend/plugins/plugin-chart-word-cloud/test/legacyPlugin/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-word-cloud/test/legacyPlugin/transformProps.test.ts index a086b139a9..a61e7408de 100644 --- a/superset-frontend/plugins/plugin-chart-word-cloud/test/legacyPlugin/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-word-cloud/test/legacyPlugin/transformProps.test.ts @@ -68,6 +68,7 @@ describe('WordCloud transformProps', () => { }, }, rotation: 'square', + colorScheme: 'bnbColors', data: [{ name: 'Hulk', sum__num: 1 }], }); }); diff --git a/superset-frontend/src/components/Chart/Chart.jsx b/superset-frontend/src/components/Chart/Chart.jsx index a77b135b30..ea1a2d968c 100644 --- a/superset-frontend/src/components/Chart/Chart.jsx +++ b/superset-frontend/src/components/Chart/Chart.jsx @@ -54,8 +54,8 @@ const propTypes = { // formData contains chart's own filter parameter // and merged with extra filter that current dashboard applying formData: PropTypes.object.isRequired, - labelColors: PropTypes.object, - sharedLabelColors: PropTypes.object, + labelsColor: PropTypes.object, + labelsColorMap: PropTypes.object, width: PropTypes.number, height: PropTypes.number, setControlValue: PropTypes.func, diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index ddedd4597b..b9cd6caf4f 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -41,8 +41,8 @@ const propTypes = { initialValues: PropTypes.object, formData: PropTypes.object.isRequired, latestQueryFormData: PropTypes.object, - labelColors: PropTypes.object, - sharedLabelColors: PropTypes.object, + labelsColor: PropTypes.object, + labelsColorMap: PropTypes.object, height: PropTypes.number, width: PropTypes.number, setControlValue: PropTypes.func, @@ -153,8 +153,8 @@ class ChartRenderer extends Component { nextProps.height !== this.props.height || nextProps.width !== this.props.width || nextProps.triggerRender || - nextProps.labelColors !== this.props.labelColors || - nextProps.sharedLabelColors !== this.props.sharedLabelColors || + nextProps.labelsColor !== this.props.labelsColor || + nextProps.labelsColorMap !== this.props.labelsColorMap || nextProps.formData.color_scheme !== this.props.formData.color_scheme || nextProps.formData.stack !== this.props.formData.stack || nextProps.cacheBusterProp !== this.props.cacheBusterProp || diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts index d1029203c7..14659b9fc3 100644 --- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts +++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts @@ -17,13 +17,7 @@ * under the License. */ import { Dispatch } from 'redux'; -import { - makeApi, - CategoricalColorNamespace, - t, - getErrorText, -} from '@superset-ui/core'; -import { isString } from 'lodash'; +import { makeApi, t, getErrorText } from '@superset-ui/core'; import { addDangerToast } from 'src/components/MessageToasts/actions'; import { ChartConfiguration, @@ -36,39 +30,8 @@ import { onSave } from './dashboardState'; export const DASHBOARD_INFO_UPDATED = 'DASHBOARD_INFO_UPDATED'; -export function updateColorSchema( - metadata: Record, - labelColors: Record, -) { - const categoricalNamespace = CategoricalColorNamespace.getNamespace( - metadata?.color_namespace, - ); - const colorMap = isString(labelColors) - ? JSON.parse(labelColors) - : labelColors; - Object.keys(colorMap).forEach(label => { - categoricalNamespace.setColor(label, colorMap[label]); - }); -} - // updates partially changed dashboard info export function dashboardInfoChanged(newInfo: { metadata: any }) { - const { metadata } = newInfo; - - const categoricalNamespace = CategoricalColorNamespace.getNamespace( - metadata?.color_namespace, - ); - - categoricalNamespace.resetColors(); - - if (metadata?.shared_label_colors) { - updateColorSchema(metadata, metadata?.shared_label_colors); - } - - if (metadata?.label_colors) { - updateColorSchema(metadata, metadata?.label_colors); - } - return { type: DASHBOARD_INFO_UPDATED, newInfo }; } export const SAVE_CHART_CONFIG_BEGIN = 'SAVE_CHART_CONFIG_BEGIN'; diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index b0e8a2719e..9b0c181821 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -23,10 +23,11 @@ import { ensureIsArray, isFeatureEnabled, FeatureFlag, - getSharedLabelColor, + getLabelsColorMap, SupersetClient, t, getClientErrorObject, + getCategoricalSchemeRegistry, } from '@superset-ui/core'; import { addChart, @@ -64,6 +65,13 @@ import { fetchDatasourceMetadata } from './datasources'; import { updateDirectPathToFilter } from './dashboardFilters'; import { SET_FILTER_CONFIG_COMPLETE } from './nativeFilters'; import getOverwriteItems from '../util/getOverwriteItems'; +import { + applyColors, + isLabelsColorMapSynced, + getLabelsColorMapEntries, + getColorSchemeDomain, + getColorNamespace, +} from '../../utils/colorScheme'; export const SET_UNSAVED_CHANGES = 'SET_UNSAVED_CHANGES'; export function setUnsavedChanges(hasUnsavedChanges) { @@ -261,7 +269,7 @@ export function saveDashboardRequest(data, id, saveType) { slug: slug || null, metadata: { ...data.metadata, - color_namespace: data.metadata?.color_namespace || undefined, + color_namespace: getColorNamespace(data.metadata?.color_namespace), color_scheme: data.metadata?.color_scheme || '', color_scheme_domain: data.metadata?.color_scheme_domain || [], expanded_slices: data.metadata?.expanded_slices || {}, @@ -584,7 +592,7 @@ export function removeSliceFromDashboard(id) { return dispatch => { dispatch(removeSlice(id)); dispatch(removeChart(id)); - getSharedLabelColor().removeSlice(id); + getLabelsColorMap().removeSlice(id); }; } @@ -657,3 +665,69 @@ export function setDatasetsStatus(status) { status, }; } + +const updateDashboardMetadata = async (id, metadata, dispatch) => { + await SupersetClient.put({ + endpoint: `/api/v1/dashboard/${id}`, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ json_metadata: JSON.stringify(metadata) }), + }); + dispatch(dashboardInfoChanged({ metadata })); +}; + +export const updateDashboardLabelsColor = () => async (dispatch, getState) => { + const { + dashboardInfo: { id, metadata }, + } = getState(); + const categoricalSchemes = getCategoricalSchemeRegistry(); + const colorScheme = metadata?.color_scheme; + const colorSchemeRegistry = categoricalSchemes.get( + metadata?.color_scheme, + true, + ); + const defaultScheme = categoricalSchemes.defaultKey; + const fallbackScheme = defaultScheme?.toString() || 'supersetColors'; + const colorSchemeDomain = metadata?.color_scheme_domain || []; + + try { + const updatedMetadata = { ...metadata }; + let updatedScheme = metadata?.color_scheme; + + // Color scheme does not exist anymore, fallback to default + if (colorScheme && !colorSchemeRegistry) { + updatedScheme = fallbackScheme; + updatedMetadata.color_scheme = updatedScheme; + updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme); + + dispatch(setColorScheme(updatedScheme)); + // must re-apply colors from fresh labels color map + applyColors(updatedMetadata, true); + } + + // stored labels color map and applied might differ + const isMapSynced = isLabelsColorMapSynced(metadata); + if (!isMapSynced) { + // re-apply a fresh labels color map + applyColors(updatedMetadata, true); + // pull and store the just applied labels color map + updatedMetadata.shared_label_colors = getLabelsColorMapEntries(); + } + + // the stored color domain registry and fresh might differ at this point + const freshColorSchemeDomain = getColorSchemeDomain(colorScheme); + const isRegistrySynced = + colorSchemeDomain.toString() !== freshColorSchemeDomain.toString(); + if (colorScheme && !isRegistrySynced) { + updatedMetadata.color_scheme_domain = freshColorSchemeDomain; + } + + if ( + (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) || + !isMapSynced + ) { + await updateDashboardMetadata(id, updatedMetadata, dispatch); + } + } catch (error) { + console.error('Failed to update dashboard color settings:', error); + } +}; diff --git a/superset-frontend/src/dashboard/actions/hydrate.js b/superset-frontend/src/dashboard/actions/hydrate.js index 7898ad0929..8dd36a8063 100644 --- a/superset-frontend/src/dashboard/actions/hydrate.js +++ b/superset-frontend/src/dashboard/actions/hydrate.js @@ -51,7 +51,6 @@ import { URL_PARAMS } from 'src/constants'; import { getUrlParam } from 'src/utils/urlUtils'; import { ResourceStatus } from 'src/hooks/apiResources/apiResources'; import extractUrlParams from '../util/extractUrlParams'; -import { updateColorSchema } from './dashboardInfo'; import updateComponentParentsList from '../util/updateComponentParentsList'; import { FilterBarOrientation } from '../types'; @@ -71,16 +70,6 @@ export const hydrateDashboard = chart.slice_id = chart.form_data.slice_id; }); - if (metadata?.shared_label_colors) { - updateColorSchema(metadata, metadata?.shared_label_colors); - } - - // Priming the color palette with user's label-color mapping provided in - // the dashboard's JSON metadata - if (metadata?.label_colors) { - updateColorSchema(metadata, metadata?.label_colors); - } - // new dash: position_json could be {} or null const layout = positionData && Object.keys(positionData).length > 0 diff --git a/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx b/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx index 30a6c4c8a5..c4204a3c1c 100644 --- a/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx +++ b/superset-frontend/src/dashboard/components/ColorSchemeControlWrapper.jsx @@ -27,11 +27,11 @@ const propTypes = { onChange: PropTypes.func, labelMargin: PropTypes.number, colorScheme: PropTypes.string, - hasCustomLabelColors: PropTypes.bool, + hasCustomLabelsColor: PropTypes.bool, }; const defaultProps = { - hasCustomLabelColors: false, + hasCustomLabelsColor: false, colorScheme: undefined, onChange: () => {}, }; @@ -50,7 +50,7 @@ class ColorSchemeControlWrapper extends PureComponent { } render() { - const { colorScheme, labelMargin = 0, hasCustomLabelColors } = this.props; + const { colorScheme, labelMargin = 0, hasCustomLabelsColor } = this.props; return ( ); } diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx index 996f377193..7c27a9f1f7 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx @@ -18,14 +18,13 @@ */ // ParentSize uses resize observer so the dashboard will update size // when its container size changes, due to e.g., builder side panel opening -import { FC, useCallback, useEffect, useMemo, useRef } from 'react'; +import { FC, useEffect, useMemo, useRef } from 'react'; import { useDispatch, useSelector } from 'react-redux'; import { Filter, Filters, - getCategoricalSchemeRegistry, - SupersetClient, - useComponentDidUpdate, + LabelsColorMapSource, + getLabelsColorMap, } from '@superset-ui/core'; import { ParentSize } from '@visx/responsive'; import { pick } from 'lodash'; @@ -44,9 +43,12 @@ import { import { getChartIdsInFilterScope } from 'src/dashboard/util/getChartIdsInFilterScope'; import findTabIndexByComponentId from 'src/dashboard/util/findTabIndexByComponentId'; import { setInScopeStatusOfFilters } from 'src/dashboard/actions/nativeFilters'; -import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; -import { setColorScheme } from 'src/dashboard/actions/dashboardState'; -import jsonStringify from 'json-stringify-pretty-compact'; +import { updateDashboardLabelsColor } from 'src/dashboard/actions/dashboardState'; +import { + applyColors, + getColorNamespace, + resetColors, +} from 'src/utils/colorScheme'; import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils'; import { findTabsWithChartsInScope } from '../nativeFilters/utils'; import { getRootLevelTabsComponent } from './utils'; @@ -131,85 +133,6 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { dispatch(setInScopeStatusOfFilters(scopes)); }, [nativeFilterScopes, dashboardLayout, dispatch]); - const verifyUpdateColorScheme = useCallback(() => { - const currentMetadata = dashboardInfo.metadata; - if (currentMetadata?.color_scheme) { - const metadata = { ...currentMetadata }; - const colorScheme = metadata?.color_scheme; - const colorSchemeDomain = metadata?.color_scheme_domain || []; - const categoricalSchemes = getCategoricalSchemeRegistry(); - const registryColorScheme = - categoricalSchemes.get(colorScheme, true) || undefined; - const registryColorSchemeDomain = registryColorScheme?.colors || []; - const defaultColorScheme = categoricalSchemes.defaultKey; - const colorSchemeExists = !!registryColorScheme; - - const updateDashboardData = () => { - SupersetClient.put({ - endpoint: `/api/v1/dashboard/${dashboardInfo.id}`, - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - json_metadata: jsonStringify(metadata), - }), - }).catch(e => console.log(e)); - }; - const updateColorScheme = (scheme: string) => { - dispatch(setColorScheme(scheme)); - }; - const updateDashboard = () => { - dispatch( - dashboardInfoChanged({ - metadata, - }), - ); - updateDashboardData(); - }; - // selected color scheme does not exist anymore - // must fallback to the available default one - if (!colorSchemeExists) { - const updatedScheme = - defaultColorScheme?.toString() || 'supersetColors'; - metadata.color_scheme = updatedScheme; - metadata.color_scheme_domain = - categoricalSchemes.get(defaultColorScheme)?.colors || []; - - // reset shared_label_colors - // TODO: Requires regenerating the shared_label_colors after - // fixing a bug which affects their generation on dashboards with tabs - metadata.shared_label_colors = {}; - - updateColorScheme(updatedScheme); - updateDashboard(); - } else { - // if this dashboard does not have a color_scheme_domain saved - // must create one and store it for the first time - if (colorSchemeExists && !colorSchemeDomain.length) { - metadata.color_scheme_domain = registryColorSchemeDomain; - updateDashboard(); - } - // if the color_scheme_domain is not the same as the registry domain - // must update the existing color_scheme_domain - if ( - colorSchemeExists && - colorSchemeDomain.length && - registryColorSchemeDomain.toString() !== colorSchemeDomain.toString() - ) { - metadata.color_scheme_domain = registryColorSchemeDomain; - - // reset shared_label_colors - // TODO: Requires regenerating the shared_label_colors after - // fixing a bug which affects their generation on dashboards with tabs - metadata.shared_label_colors = {}; - - updateColorScheme(colorScheme); - updateDashboard(); - } - } - } - }, [chartIds]); - - useComponentDidUpdate(verifyUpdateColorScheme); - const childIds: string[] = topLevelTabs ? topLevelTabs.children : [DASHBOARD_GRID_ID]; @@ -217,6 +140,29 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { const activeKey = min === 0 ? DASHBOARD_GRID_ID : min.toString(); const TOP_OF_PAGE_RANGE = 220; + useEffect(() => { + // verify freshness of color map on tab change + // and when loading for first time + setTimeout(() => { + dispatch(updateDashboardLabelsColor()); + }, 500); + }, [directPathToChild, dispatch]); + + useEffect(() => { + const labelsColorMap = getLabelsColorMap(); + const colorNamespace = getColorNamespace( + dashboardInfo?.metadata?.color_namespace, + ); + labelsColorMap.source = LabelsColorMapSource.Dashboard; + // apply labels color as dictated by stored metadata + applyColors(dashboardInfo.metadata); + + return () => { + resetColors(getColorNamespace(colorNamespace)); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [dashboardInfo.id, dispatch]); + return (
diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index bb0f79d1d4..d9f5dbd6d0 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -26,7 +26,6 @@ import { isFeatureEnabled, FeatureFlag, t, - getSharedLabelColor, getExtensionsRegistry, } from '@superset-ui/core'; import { Global } from '@emotion/react'; @@ -374,13 +373,10 @@ class Header extends PureComponent { ? currentRefreshFrequency : dashboardInfo.metadata?.refresh_frequency; - const currentColorScheme = - dashboardInfo?.metadata?.color_scheme || colorScheme; const currentColorNamespace = dashboardInfo?.metadata?.color_namespace || colorNamespace; - const currentSharedLabelColors = Object.fromEntries( - getSharedLabelColor().getColorMap(), - ); + const currentColorScheme = + dashboardInfo?.metadata?.color_scheme || colorScheme; const data = { certified_by: dashboardInfo.certified_by, @@ -397,7 +393,6 @@ class Header extends PureComponent { color_scheme: currentColorScheme, positions, refresh_frequency: refreshFrequency, - shared_label_colors: currentSharedLabelColors, }, }; diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 1f2b6fa5d8..8fef965097 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -25,12 +25,10 @@ import Button from 'src/components/Button'; import { AntdForm, AsyncSelect, Col, Row } from 'src/components'; import rison from 'rison'; import { - CategoricalColorNamespace, ensureIsArray, isFeatureEnabled, FeatureFlag, getCategoricalSchemeRegistry, - getSharedLabelColor, styled, SupersetClient, t, @@ -46,6 +44,7 @@ import withToasts from 'src/components/MessageToasts/withToasts'; import TagType from 'src/types/TagType'; import { fetchTags, OBJECT_TYPES } from 'src/features/tags/tags'; import { loadTags } from 'src/components/Tags/utils'; +import { applyColors, getColorNamespace } from 'src/utils/colorScheme'; const StyledFormItem = styled(FormItem)` margin-bottom: 0; @@ -307,7 +306,6 @@ const PropertiesModal = ({ const { title, slug, certifiedBy, certificationDetails } = form.getFieldsValue(); let currentColorScheme = colorScheme; - let colorNamespace = ''; let currentJsonMetadata = jsonMetadata; // validate currentJsonMetadata @@ -325,11 +323,13 @@ const PropertiesModal = ({ return; } + const copyMetadata = { ...metadata }; + const colorNamespace = getColorNamespace(metadata?.color_namespace); + // color scheme in json metadata has precedence over selection currentColorScheme = metadata?.color_scheme || colorScheme; - colorNamespace = metadata?.color_namespace; - // filter shared_label_color from user input + // remove information from user facing input if (metadata?.shared_label_colors) { delete metadata.shared_label_colors; } @@ -337,22 +337,8 @@ const PropertiesModal = ({ delete metadata.color_scheme_domain; } - 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 = []; - } + // only apply colors, the user has not saved yet + applyColors(copyMetadata, true); currentJsonMetadata = jsonStringify(metadata); @@ -410,7 +396,7 @@ const PropertiesModal = ({ const getRowsWithoutRoles = () => { const jsonMetadataObj = getJsonMetadata(); - const hasCustomLabelColors = !!Object.keys( + const hasCustomLabelsColor = !!Object.keys( jsonMetadataObj?.label_colors || {}, ).length; @@ -440,7 +426,7 @@ const PropertiesModal = ({

{t('Colors')}

{ const jsonMetadataObj = getJsonMetadata(); - const hasCustomLabelColors = !!Object.keys( + const hasCustomLabelsColor = !!Object.keys( jsonMetadataObj?.label_colors || {}, ).length; @@ -509,7 +495,7 @@ const PropertiesModal = ({ = ({ dashboardPageId }) => { DashboardContextForExplore >( ({ dashboardInfo, dashboardState, nativeFilters, dataMask }) => ({ - labelColors: dashboardInfo.metadata?.label_colors || EMPTY_OBJECT, - sharedLabelColors: + labelsColor: dashboardInfo.metadata?.label_colors || EMPTY_OBJECT, + labelsColorMap: dashboardInfo.metadata?.shared_label_colors || EMPTY_OBJECT, colorScheme: dashboardState?.colorScheme, chartConfiguration: diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index bce0fedcaa..f519c8f83d 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -54,8 +54,8 @@ const propTypes = { // from redux chart: chartPropShape.isRequired, formData: PropTypes.object.isRequired, - labelColors: PropTypes.object, - sharedLabelColors: PropTypes.object, + labelsColor: PropTypes.object, + labelsColorMap: PropTypes.object, datasource: PropTypes.object, slice: slicePropShape.isRequired, sliceName: PropTypes.string.isRequired, @@ -382,8 +382,8 @@ class Chart extends Component { editMode, filters, formData, - labelColors, - sharedLabelColors, + labelsColor, + labelsColorMap, updateSliceName, sliceName, toggleExpandSlice, @@ -509,8 +509,8 @@ class Chart extends Component { dashboardId={dashboardId} initialValues={initialValues} formData={formData} - labelColors={labelColors} - sharedLabelColors={sharedLabelColors} + labelsColor={labelsColor} + labelsColorMap={labelsColorMap} ownState={ownState} filterState={filterState} queriesResponse={chart.queriesResponse} diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx index 491befef22..2b47eb7bb3 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx @@ -429,4 +429,5 @@ function mapStateToProps(state) { directPathToChild: state.dashboardState.directPathToChild, }; } + export default connect(mapStateToProps)(Tabs); diff --git a/superset-frontend/src/dashboard/containers/Chart.jsx b/superset-frontend/src/dashboard/containers/Chart.jsx index 1e50602b06..6451cf2004 100644 --- a/superset-frontend/src/dashboard/containers/Chart.jsx +++ b/superset-frontend/src/dashboard/containers/Chart.jsx @@ -60,8 +60,8 @@ function mapStateToProps( (chart && chart.form_data && datasources[chart.form_data.datasource]) || PLACEHOLDER_DATASOURCE; const { colorScheme, colorNamespace, datasetsStatus } = dashboardState; - const labelColors = dashboardInfo?.metadata?.label_colors || {}; - const sharedLabelColors = dashboardInfo?.metadata?.shared_label_colors || {}; + const labelsColor = dashboardInfo?.metadata?.label_colors || {}; + const labelsColorMap = dashboardInfo?.metadata?.shared_label_colors || {}; // note: this method caches filters if possible to prevent render cascades const formData = getFormDataWithExtraFilters({ chart, @@ -75,8 +75,8 @@ function mapStateToProps( allSliceIds: dashboardState.sliceIds, dataMask, extraControls, - labelColors, - sharedLabelColors, + labelsColor, + labelsColorMap, }); formData.dashboardId = dashboardInfo.id; @@ -84,8 +84,8 @@ function mapStateToProps( return { chart, datasource, - labelColors, - sharedLabelColors, + labelsColor, + labelsColorMap, slice: sliceEntities.slices[id], timeout: dashboardInfo.common.conf.SUPERSET_WEBSERVER_TIMEOUT, filters: getActiveFilters() || EMPTY_OBJECT, diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx index 23e8534d96..bf92c5dcec 100644 --- a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx +++ b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx @@ -78,6 +78,7 @@ function mapStateToProps( editMode: dashboardState.editMode, filters: getActiveFilters(), dashboardId: dashboardInfo.id, + dashboardInfo, fullSizeChartId: dashboardState.fullSizeChartId, }; diff --git a/superset-frontend/src/dashboard/containers/DashboardPage.tsx b/superset-frontend/src/dashboard/containers/DashboardPage.tsx index 9edf14e759..655541756d 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -19,13 +19,7 @@ import { createContext, lazy, FC, useEffect, useMemo, useRef } from 'react'; import { Global } from '@emotion/react'; import { useHistory } from 'react-router-dom'; -import { - CategoricalColorNamespace, - getSharedLabelColor, - SharedLabelColorSource, - t, - useTheme, -} from '@superset-ui/core'; +import { t, useTheme } from '@superset-ui/core'; import { useDispatch, useSelector } from 'react-redux'; import { useToasts } from 'src/components/MessageToasts/withToasts'; import Loading from 'src/components/Loading'; @@ -101,7 +95,7 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { const error = dashboardApiError || chartsApiError; const readyToRender = Boolean(dashboard && charts); - const { dashboard_title, css, metadata, id = 0 } = dashboard || {}; + const { dashboard_title, css, id = 0 } = dashboard || {}; useEffect(() => { // mark tab id as redundant when user closes browser tab - a new id will be @@ -187,19 +181,6 @@ export const DashboardPage: FC = ({ idOrSlug }: PageProps) => { return () => {}; }, [css]); - useEffect(() => { - const sharedLabelColor = getSharedLabelColor(); - sharedLabelColor.source = SharedLabelColorSource.Dashboard; - return () => { - // clean up label color - const categoricalNamespace = CategoricalColorNamespace.getNamespace( - metadata?.color_namespace, - ); - categoricalNamespace.resetColors(); - sharedLabelColor.clear(); - }; - }, [metadata?.color_namespace]); - useEffect(() => { if (datasetsApiError) { addDangerToast( diff --git a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts index 0b7c6a9d7d..9160d21e58 100644 --- a/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts +++ b/superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts @@ -43,8 +43,8 @@ export interface GetFormDataWithExtraFiltersArguments { dataMask: DataMaskStateWithId; nativeFilters: PartialFilters; extraControls: Record; - labelColors?: Record; - sharedLabelColors?: Record; + labelsColor?: Record; + labelsColorMap?: Record; allSliceIds: number[]; } @@ -61,8 +61,8 @@ export default function getFormDataWithExtraFilters({ sliceId, dataMask, extraControls, - labelColors, - sharedLabelColors, + labelsColor, + labelsColorMap, allSliceIds, }: GetFormDataWithExtraFiltersArguments) { // if dashboard metadata + filters have not changed, use cache if possible @@ -75,10 +75,10 @@ export default function getFormDataWithExtraFilters({ areObjectsEqual(cachedFormData?.color_namespace, colorNamespace, { ignoreUndefined: true, }) && - areObjectsEqual(cachedFormData?.label_colors, labelColors, { + areObjectsEqual(cachedFormData?.label_colors, labelsColor, { ignoreUndefined: true, }) && - areObjectsEqual(cachedFormData?.shared_label_colors, sharedLabelColors, { + areObjectsEqual(cachedFormData?.shared_label_colors, labelsColorMap, { ignoreUndefined: true, }) && !!cachedFormData && @@ -110,8 +110,8 @@ export default function getFormDataWithExtraFilters({ const formData = { ...chart.form_data, - label_colors: labelColors, - shared_label_colors: sharedLabelColors, + label_colors: labelsColor, + shared_label_colors: labelsColorMap, ...(colorScheme && { color_scheme: colorScheme }), extra_filters: getEffectiveExtraFilters(filters), ...extraData, diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index 990437a6d0..988b11f825 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -21,14 +21,7 @@ import { useHistory } from 'react-router-dom'; import { useDispatch } from 'react-redux'; import PropTypes from 'prop-types'; import { Tooltip } from 'src/components/Tooltip'; -import { - CategoricalColorNamespace, - css, - logging, - SupersetClient, - t, - tn, -} from '@superset-ui/core'; +import { css, logging, SupersetClient, t, tn } from '@superset-ui/core'; import { chartPropShape } from 'src/dashboard/util/propShapes'; import AlteredSliceTag from 'src/components/AlteredSliceTag'; import Button from 'src/components/Button'; @@ -38,6 +31,7 @@ import { sliceUpdated } from 'src/explore/actions/exploreActions'; import { PageHeaderWithActions } from 'src/components/PageHeaderWithActions'; import MetadataBar, { MetadataType } from 'src/components/MetadataBar'; import { setSaveChartModalVisibility } from 'src/explore/actions/saveModalActions'; +import { applyColors, resetColors } from 'src/utils/colorScheme'; import { useExploreAdditionalActionsMenu } from '../useExploreAdditionalActionsMenu'; const propTypes = { @@ -96,6 +90,13 @@ export const ExploreChartHeader = ({ const dashboard = dashboardId && dashboards && dashboards.find(d => d.id === dashboardId); + if (!dashboard) { + // clean up color namespace and shared color maps + // to avoid colors spill outside of dashboard context + resetColors(metadata?.color_namespace); + return; + } + if (dashboard) { try { // Dashboards from metadata don't contain the json_metadata field @@ -106,23 +107,8 @@ export const ExploreChartHeader = ({ const result = response?.json?.result; // setting the chart to use the dashboard custom label colors if any - const metadata = JSON.parse(result.json_metadata); - const sharedLabelColors = metadata.shared_label_colors || {}; - const customLabelColors = metadata.label_colors || {}; - const mergedLabelColors = { - ...sharedLabelColors, - ...customLabelColors, - }; - - const categoricalNamespace = CategoricalColorNamespace.getNamespace(); - - Object.keys(mergedLabelColors).forEach(label => { - categoricalNamespace.setColor( - label, - mergedLabelColors[label], - metadata.color_scheme, - ); - }); + const dashboardMetadata = JSON.parse(result.json_metadata); + applyColors(dashboardMetadata); } catch (error) { logging.info(t('Unable to retrieve dashboard colors')); } @@ -130,7 +116,7 @@ export const ExploreChartHeader = ({ }; useEffect(() => { - if (dashboardId) updateCategoricalNamespace(); + updateCategoricalNamespace(); }, []); const openPropertiesModal = () => { diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeControl.test.tsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeControl.test.tsx index 808151b625..0f380b3910 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeControl.test.tsx @@ -28,7 +28,7 @@ import { render, screen, waitFor } from 'spec/helpers/testing-library'; import ColorSchemeControl, { ColorSchemes } from '.'; const defaultProps = () => ({ - hasCustomLabelColors: false, + hasCustomLabelsColor: false, label: 'Color scheme', labelMargin: 0, name: 'color', @@ -58,7 +58,7 @@ test('should display a label', async () => { expect(await screen.findByText('Color scheme')).toBeTruthy(); }); -test('should not display an alert icon if hasCustomLabelColors=false', async () => { +test('should not display an alert icon if hasCustomLabelsColor=false', async () => { setup(); await waitFor(() => { expect( @@ -67,11 +67,12 @@ test('should not display an alert icon if hasCustomLabelColors=false', async () }); }); -test('should display an alert icon if hasCustomLabelColors=true', async () => { - const hasCustomLabelColorsProps = { - hasCustomLabelColors: true, +test('should display an alert icon if hasCustomLabelsColor=true', async () => { + const hasCustomLabelsColorProps = { + ...defaultProps, + hasCustomLabelsColor: true, }; - setup(hasCustomLabelColorsProps); + setup(hasCustomLabelsColorProps); await waitFor(() => { expect( screen.getByRole('img', { name: 'alert-solid' }), diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx index 74c4374dae..a14df21a22 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/ColorSchemeLabel.tsx @@ -31,17 +31,17 @@ export default function ColorSchemeLabel(props: ColorSchemeLabelProps) { const { id, label, colors } = props; const [showTooltip, setShowTooltip] = useState(false); const labelNameRef = useRef(null); - const labelColorsRef = useRef(null); + const labelsColorRef = useRef(null); const handleShowTooltip = () => { const labelNameElement = labelNameRef.current; - const labelColorsElement = labelColorsRef.current; + const labelsColorElement = labelsColorRef.current; if ( labelNameElement && - labelColorsElement && + labelsColorElement && (labelNameElement.scrollWidth > labelNameElement.offsetWidth || labelNameElement.scrollHeight > labelNameElement.offsetHeight || - labelColorsElement.scrollWidth > labelColorsElement.offsetWidth || - labelColorsElement.scrollHeight > labelColorsElement.offsetHeight) + labelsColorElement.scrollWidth > labelsColorElement.offsetWidth || + labelsColorElement.scrollHeight > labelsColorElement.offsetHeight) ) { setShowTooltip(true); } @@ -109,7 +109,7 @@ export default function ColorSchemeLabel(props: ColorSchemeLabelProps) { {label} css` flex: 100%; text-overflow: ellipsis; diff --git a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.tsx b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.tsx index f4178beab2..e43bb32b72 100644 --- a/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.tsx +++ b/superset-frontend/src/explore/components/controls/ColorSchemeControl/index.tsx @@ -46,7 +46,7 @@ export interface ColorSchemes { } export interface ColorSchemeControlProps { - hasCustomLabelColors: boolean; + hasCustomLabelsColor: boolean; dashboardId?: number; label: string; name: string; @@ -75,14 +75,14 @@ const DASHBOARD_ALERT = t( const Label = ({ label, - hasCustomLabelColors, + hasCustomLabelsColor, dashboardId, }: Pick< ColorSchemeControlProps, - 'label' | 'hasCustomLabelColors' | 'dashboardId' + 'label' | 'hasCustomLabelsColor' | 'dashboardId' >) => { - if (hasCustomLabelColors || dashboardId) { - const alertTitle = hasCustomLabelColors + if (hasCustomLabelsColor || dashboardId) { + const alertTitle = hasCustomLabelsColor ? CUSTOM_LABEL_ALERT : DASHBOARD_ALERT; return ( @@ -98,7 +98,7 @@ const Label = ({ }; const ColorSchemeControl = ({ - hasCustomLabelColors = false, + hasCustomLabelsColor = false, dashboardId, label = t('Color scheme'), onChange = () => {}, @@ -231,7 +231,7 @@ const ColorSchemeControl = ({ label={