This is an automated email from the ASF dual-hosted git repository. diegopucci pushed a commit to branch diego/ch79154/fix-color-inconsistency in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6f89a9ca0e0808cdf0b9a564db65f09c9e20d11e Author: geido <[email protected]> AuthorDate: Fri Mar 8 16:08:34 2024 +0100 Update map on hidden tabs --- RESOURCES/FEATURE_FLAGS.md | 1 + .../cypress/e2e/dashboard/editmode.test.ts | 56 ++++--- .../src/color/CategoricalColorScale.ts | 149 +++++++++++------- .../src/color/SharedLabelColorSingleton.ts | 19 +-- .../test/color/CategoricalColorNameSpace.test.ts | 2 +- .../test/color/CategoricalColorScale.test.ts | 167 ++++++++++----------- .../test/color/SharedLabelColorSingleton.test.ts | 16 +- .../src/dashboard/actions/dashboardInfo.ts | 13 +- .../src/dashboard/components/Header/index.jsx | 10 +- .../dashboard/components/PropertiesModal/index.tsx | 7 +- .../dashboard/components/gridComponents/Tabs.jsx | 51 ++++++- .../dashboard/containers/DashboardComponent.jsx | 1 + .../src/dashboard/containers/DashboardPage.tsx | 1 + .../components/ExploreChartHeader/index.jsx | 23 ++- tests/integration_tests/superset_test_config.py | 1 + 15 files changed, 296 insertions(+), 221 deletions(-) diff --git a/RESOURCES/FEATURE_FLAGS.md b/RESOURCES/FEATURE_FLAGS.md index 119988653d..1bbf24f4be 100644 --- a/RESOURCES/FEATURE_FLAGS.md +++ b/RESOURCES/FEATURE_FLAGS.md @@ -84,6 +84,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 28d5c166f5..e50bf52be9 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -19,7 +19,12 @@ import { SAMPLE_DASHBOARD_1, TABBED_DASHBOARD } from 'cypress/utils/urls'; import { drag, resize, waitForChartLoad } from 'cypress/utils'; import * as ace from 'brace'; -import { interceptGet, interceptUpdate, openTab } from './utils'; +import { + interceptGet, + interceptUpdate, + openTab, + interceptGet as interceptDashboardGet, +} from './utils'; import { interceptExploreJson, interceptFiltering as interceptCharts, @@ -124,7 +129,7 @@ function selectColorScheme(color: string) { ) .first() .click(); - cy.getBySel(color).click(); + cy.getBySel(color).click({ force: true }); } function applyChanges() { @@ -169,6 +174,7 @@ function writeMetadata(metadata: string) { function openExplore(chartName: string) { interceptExploreJson(); + interceptDashboardGet(); cy.get( `[data-test-chart-name='${chartName}'] [aria-label='More Options']`, @@ -210,7 +216,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 +237,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); @@ -240,7 +246,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)'); }); it('should apply same color to same labels with no color scheme set', () => { @@ -261,7 +267,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 +276,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 +390,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 +429,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 +465,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 +472,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('should change color scheme multiple times', () => { @@ -496,7 +492,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 +501,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 +512,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 +523,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('should apply the color scheme across main tabs', () => { @@ -542,7 +538,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('should apply the color scheme across main tabs for rendered charts', () => { @@ -558,7 +554,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 +584,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/packages/superset-ui-core/src/color/CategoricalColorScale.ts b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts index c4e96a9a1f..4223fae776 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts @@ -17,11 +17,13 @@ class CategoricalColorScale extends ExtensibleFunction { colors: string[]; + expandedColors: string[]; + scale: ScaleOrdinal<{ toString(): string }, string>; forcedColors: ColorsLookup; - colorUsageCount: Map<string, number>; + sharedColorMapInstance: ReturnType<typeof getSharedLabelColor>; sliceMap: Map<string, string>; @@ -35,8 +37,17 @@ class CategoricalColorScale extends ExtensibleFunction { */ constructor(colors: string[], forcedColors: ColorsInitLookup = {}) { super((value: string, sliceId?: number) => this.getColor(value, sliceId)); + // holds original color scheme colors this.originColors = colors; - this.colors = colors; + // holds the extended color range (includes analagous colors) + this.expandedColors = colors; + // holds the values of this specific slice (label+color) + this.sliceMap = new Map(); + // shared color map instance (when context is shared, i.e. dashboard) + this.sharedColorMapInstance = getSharedLabelColor(); + // holds the multiple value for analogous colors range + this.multiple = 0; + this.scale = scaleOrdinal<{ toString(): string }, string>(); this.scale.range(colors); @@ -49,18 +60,12 @@ class CategoricalColorScale extends ExtensibleFunction { }); // forced colors from parent (usually CategoricalColorNamespace) + // currently used in dashboards to set custom label colors this.forcedColors = forcedColors as ColorsLookup; - // holds the usage count for each color including forced colors from parent - this.colorUsageCount = this.initColorUsageCounter(); - // holds the values in this specific slice - this.sliceMap = new Map(); - // holds the multiple value for analogous colors range - this.multiple = 0; } /** * Increments the color range with analogous colors - * */ incrementColorRange() { const multiple = Math.floor( @@ -71,34 +76,24 @@ class CategoricalColorScale extends ExtensibleFunction { if (multiple > this.multiple) { this.multiple = multiple; const newRange = getAnalogousColors(this.originColors, multiple); - this.range(this.originColors.concat(newRange)); - // update the colorUsageCount map with the new colors - newRange.forEach(color => { - this.colorUsageCount.set(color, 0); - }); + const expandedColors = this.originColors.concat(newRange); + + this.range(expandedColors); + this.expandedColors = expandedColors; } } /** - * Initializes the color usage count map - * @returns a map of color to usage count + * Get the color for a given value + * @param value + * @param sliceId + * @returns the color for the given value */ - initColorUsageCounter(): Map<string, number> { - const colorUsageCount = new Map(this.colors.map(color => [color, 0])); - // add forced colors to the usage count map - // helps reducing conflicts but increases randomness - Object.values(this.forcedColors).forEach(color => { - const count = colorUsageCount.get(color) || 0; - colorUsageCount.set(color, count + 1); - }); - return colorUsageCount; - } - getColor(value?: string, sliceId?: number): string { const cleanedValue = stringifyAndTrim(value); - const sharedLabelColor = getSharedLabelColor(); - const sharedColorMap = sharedLabelColor.getColorMap(); + const sharedColorMap = this.sharedColorMapInstance.getColorMap(); const sharedColor = sharedColorMap.get(cleanedValue); + // priority: forced color (example: custom label colors) > shared color > scale color const forcedColor = this.forcedColors?.[cleanedValue] || sharedColor; let color = forcedColor || this.scale(cleanedValue); @@ -107,22 +102,24 @@ class CategoricalColorScale extends ExtensibleFunction { if (isFeatureEnabled(FeatureFlag.UseAnalagousColors)) { this.incrementColorRange(); } - if (this.isColorUsed(color)) { - // color was used in this slice already + 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); } } - // increment the usage count for the color - this.incrementColorUsage(color); - - // store the value+color in the shared map for dashboard consistency - sharedLabelColor.addSlice(cleanedValue, color, sliceId); - // keep track of values in this slice this.sliceMap.set(cleanedValue, color); + // store the value+color in the shared context + if (sliceId) { + this.sharedColorMapInstance.addSlice(cleanedValue, color, sliceId); + } + return color; } @@ -132,30 +129,69 @@ class CategoricalColorScale extends ExtensibleFunction { * @returns whether the color is used in this slice */ isColorUsed(color: string): boolean { - return Array.from(this.sliceMap.values()).includes(color); + return this.getColorUsageCount(color) > 0; + } + + /** + * Get the count of the color usage in this slice + * @param sliceId + * @param color + * @returns the count of the color usage in this slice + */ + getColorUsageCount(currentColor: string): number { + let count = 0; + this.sliceMap.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 and forced colors (all slices from parent) + * Checks across colors of current slice within SharedLabelColorSingleton * - * @param excludeColor + * @param currentColor * @returns the least used color that is not the excluded color */ - getNextAvailableColor(excludeColor: string) { - // sort the colors by count, then exclude the specified color - const sortedColors = [...this.colorUsageCount.entries()] - .sort((a, b) => a[1] - b[1]) - // eslint-disable-next-line @typescript-eslint/no-unused-vars - .filter(([color, _]) => color !== excludeColor); - const color = sortedColors[0][0]; + getNextAvailableColor(currentColor: string) { + const colorUsageArray = this.expandedColors.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, + ); - return color; + // 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; } - incrementColorUsage(color: string) { - const currentCount = this.colorUsageCount.get(color) || 0; - this.colorUsageCount.set(color, currentCount + 1); + /** + * 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 + */ + setColor(value: string, forcedColor: string) { + this.forcedColors[stringifyAndTrim(value)] = forcedColor; + return this; } /** @@ -163,11 +199,15 @@ class CategoricalColorScale extends ExtensibleFunction { * @returns an object where the key is the data value and the value is the hex color code */ getColorMap() { - const colorMap = {}; + const colorMap: { [key: string]: string | undefined } = {}; this.scale.domain().forEach(value => { colorMap[value.toString()] = this.scale(value); }); - return { ...colorMap, ...this.forcedColors }; + + return { + ...colorMap, + ...this.forcedColors, + }; } /** @@ -179,7 +219,6 @@ class CategoricalColorScale extends ExtensibleFunction { this.forcedColors, ); copy.forcedColors = { ...this.forcedColors }; - copy.colorUsageCount = new Map(this.colorUsageCount); copy.domain(this.domain()); copy.unknown(this.unknown()); return copy; @@ -225,7 +264,7 @@ class CategoricalColorScale extends ExtensibleFunction { return this.scale.range(); } - this.colors = newRange; + this.expandedColors = newRange; this.scale.range(newRange); return this; } diff --git a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts index 4837d0f362..05a56de129 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/SharedLabelColorSingleton.ts @@ -43,10 +43,10 @@ export class SharedLabelColor { CategoricalColorNamespace.getNamespace(colorNamespace); const newColorMap = new Map(); this.colorMap.clear(); - this.sliceLabelMap.forEach(labels => { + this.sliceLabelMap.forEach((labels, sliceId) => { const colorScale = categoricalNamespace.getScale(colorScheme); labels.forEach(label => { - const newColor = colorScale(label); + const newColor = colorScale.getColor(label, sliceId); newColorMap.set(label, newColor); }); }); @@ -57,12 +57,8 @@ export class SharedLabelColor { return this.colorMap; } - addSlice(label: string, color: string, sliceId?: number) { - if ( - this.source !== SharedLabelColorSource.Dashboard || - sliceId === undefined - ) - return; + addSlice(label: string, color: string, sliceId: number) { + if (this.source !== SharedLabelColorSource.Dashboard) return; const labels = this.sliceLabelMap.get(sliceId) || []; if (!labels.includes(label)) { labels.push(label); @@ -83,13 +79,6 @@ 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.colorMap.clear(); 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..987c83fbc5 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,49 @@ */ import { ScaleOrdinal } from 'd3-scale'; -import { - CategoricalColorScale, - FeatureFlag, - getSharedLabelColor, -} from '@superset-ui/core'; +import { CategoricalColorScale, FeatureFlag } from '@superset-ui/core'; describe('CategoricalColorScale', () => { 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)', () => { 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'); @@ -118,26 +117,8 @@ describe('CategoricalColorScale', () => { scale.getColor('cow'); scale.getColor('donkey'); scale.getColor('goat'); - expect(scale.range()).toHaveLength(6); + expect(scale.range()).toHaveLength(9); }); - - it('should remove shared color from range if avoid colors collision enabled', () => { - 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); - }); - window.featureFlags = { - [FeatureFlag.AvoidColorsCollision]: false, - }; }); describe('.setColor(value, forcedColor)', () => { it('overrides default color', () => { @@ -145,16 +126,14 @@ describe('CategoricalColorScale', () => { 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 +142,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 +156,7 @@ describe('CategoricalColorScale', () => { expect(scale2.getColorMap()).toEqual({ cow: 'black', pig: 'pink', - horse: 'green', + horse: 'blue', // least used color }); }); }); @@ -230,64 +209,72 @@ 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 least used color', () => { + window.featureFlags = { + [FeatureFlag.AvoidColorsCollision]: true, + }; 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('red'); // blue is now used and red is next available + expect(scale.getColor('cat')).toBe('blue'); // pig got red, now blue is available + expect(scale('cat')).toBe('green'); // blue is now used and green is next available }); - }); - - describe("is compatible with D3's ScaleOrdinal", () => { - it('passes type check', () => { - const scale: ScaleOrdinal<{ toString(): string }, string> = - new CategoricalColorScale(['blue', 'red', 'green']); + it('scale(value) returns same color for same value', () => { + window.featureFlags = { + [FeatureFlag.AvoidColorsCollision]: false, + }; + const scale = new CategoricalColorScale(['blue', 'red', 'green']); + expect(scale.getColor('pig')).toBe('blue'); expect(scale('pig')).toBe('blue'); + expect(scale.getColor('cat')).toBe('red'); + expect(scale('cat')).toBe('red'); }); }); - 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']); + 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'); - 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(); + // Since 'green' hasn't been used, it's considered the least used. + expect(scale.getNextAvailableColor('blue')).toBe('green'); }); - 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('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('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(); + 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("is compatible with D3's ScaleOrdinal", () => { + it('passes type check', () => { + const scale: ScaleOrdinal<{ toString(): string }, string> = + new CategoricalColorScale(['blue', 'red', 'green']); + expect(scale('pig')).toBe('blue'); }); }); }); diff --git a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts index 8f89ae8a69..ae23f34cc4 100644 --- a/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/color/SharedLabelColorSingleton.test.ts @@ -96,13 +96,7 @@ describe('SharedLabelColor', () => { 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'); + sharedLabelColor.addSlice('a', 'red', 1); expect(Object.fromEntries(sharedLabelColor.sliceLabelMap)).toEqual({}); }); }); @@ -144,8 +138,8 @@ describe('SharedLabelColor', () => { const colorMap = sharedLabelColor.getColorMap(); expect(Object.fromEntries(colorMap)).toEqual({ a: 'yellow', - b: 'yellow', - c: 'green', + b: 'green', + c: 'yellow', }); }); @@ -195,9 +189,9 @@ describe('SharedLabelColor', () => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.addSlice('a', 'red', 1); sharedLabelColor.addSlice('b', 'blue', 2); - sharedLabelColor.reset(); + sharedLabelColor.clear(); const colorMap = sharedLabelColor.getColorMap(); - expect(Object.fromEntries(colorMap)).toEqual({ a: '', b: '' }); + expect(Object.fromEntries(colorMap)).toEqual({}); }); }); }); diff --git a/superset-frontend/src/dashboard/actions/dashboardInfo.ts b/superset-frontend/src/dashboard/actions/dashboardInfo.ts index 472f945b54..36c2f02f61 100644 --- a/superset-frontend/src/dashboard/actions/dashboardInfo.ts +++ b/superset-frontend/src/dashboard/actions/dashboardInfo.ts @@ -54,16 +54,15 @@ export function dashboardInfoChanged(newInfo: { metadata: any }) { const categoricalNamespace = CategoricalColorNamespace.getNamespace( metadata?.color_namespace, ); - + // reset forced colors categoricalNamespace.resetColors(); - if (metadata?.shared_label_colors) { - updateColorSchema(metadata, metadata?.shared_label_colors); - } + const mergedLabelColors = { + ...(metadata?.label_colors || {}), + ...(metadata?.shared_label_colors || {}), + }; - if (metadata?.label_colors) { - updateColorSchema(metadata, metadata?.label_colors); - } + updateColorSchema(metadata, mergedLabelColors); return { type: DASHBOARD_INFO_UPDATED, newInfo }; } diff --git a/superset-frontend/src/dashboard/components/Header/index.jsx b/superset-frontend/src/dashboard/components/Header/index.jsx index 485185d607..e924d22770 100644 --- a/superset-frontend/src/dashboard/components/Header/index.jsx +++ b/superset-frontend/src/dashboard/components/Header/index.jsx @@ -372,13 +372,17 @@ class Header extends React.PureComponent { ? currentRefreshFrequency : dashboardInfo.metadata?.refresh_frequency; - const currentColorScheme = - dashboardInfo?.metadata?.color_scheme || colorScheme; const currentColorNamespace = dashboardInfo?.metadata?.color_namespace || colorNamespace; + const currentColorScheme = + dashboardInfo?.metadata?.color_scheme || colorScheme; const currentSharedLabelColors = Object.fromEntries( getSharedLabelColor().getColorMap(), ); + // an empty color scheme should not bring shared label colors forward + const sharedLabelColors = currentColorScheme + ? currentSharedLabelColors + : {}; const data = { certified_by: dashboardInfo.certified_by, @@ -395,7 +399,7 @@ class Header extends React.PureComponent { color_scheme: currentColorScheme, positions, refresh_frequency: refreshFrequency, - shared_label_colors: currentSharedLabelColors, + shared_label_colors: sharedLabelColors, }, }; diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx index 1b5ef5fceb..d1ebc3d84e 100644 --- a/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx +++ b/superset-frontend/src/dashboard/components/PropertiesModal/index.tsx @@ -381,16 +381,21 @@ const PropertiesModal = ({ const sharedLabelColor = getSharedLabelColor(); const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNamespace); + + // reset forced colors categoricalNamespace.resetColors(); + if (currentColorScheme) { + // reset the shared label color map based on the current color scheme sharedLabelColor.updateColorMap(colorNamespace, currentColorScheme); + // store the shared label color map and domain in the metadata metadata.shared_label_colors = Object.fromEntries( sharedLabelColor.getColorMap(), ); metadata.color_scheme_domain = categoricalSchemeRegistry.get(colorScheme)?.colors || []; } else { - sharedLabelColor.reset(); + sharedLabelColor.clear(); metadata.shared_label_colors = {}; metadata.color_scheme_domain = []; } diff --git a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx index c9f35d3ba5..fc0bf5aabd 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Tabs.jsx @@ -18,11 +18,19 @@ */ import React from 'react'; import PropTypes from 'prop-types'; -import { styled, t } from '@superset-ui/core'; +import { + getSharedLabelColor, + styled, + SupersetClient, + t, +} from '@superset-ui/core'; import { connect } from 'react-redux'; import { LineEditableTabs } from 'src/components/Tabs'; import { LOG_ACTIONS_SELECT_DASHBOARD_TAB } from 'src/logger/LogUtils'; import { AntdModal } from 'src/components'; +import { isEqual } from 'lodash'; +import jsonStringify from 'json-stringify-pretty-compact'; +import { dashboardInfoChanged } from 'src/dashboard/actions/dashboardInfo'; import { Draggable } from '../dnd/DragDroppable'; import DragHandle from '../dnd/DragHandle'; import DashboardComponent from '../../containers/DashboardComponent'; @@ -256,10 +264,51 @@ export class Tabs extends React.PureComponent { } }; + // the initial color map is generated on save and catches all rendered charts + // charts hidden in nested tabs are not rendered, thus not included + // must update the label color map when switching tabs to include these charts + updateLabelColorsMap() { + const currentLabelColorsMap = Object.fromEntries( + getSharedLabelColor().getColorMap(), + ); + const { metadata } = this.props.dashboardInfo; + const labelColorsMap = metadata?.shared_label_colors || {}; + // color consistency is currently only supported when a color scheme is set + if ( + metadata?.color_scheme && + !isEqual(labelColorsMap, currentLabelColorsMap) + ) { + const updatedMetadata = { + ...metadata, + shared_label_colors: currentLabelColorsMap, + }; + SupersetClient.put({ + endpoint: `/api/v1/dashboard/${this.props.dashboardId}`, + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + json_metadata: jsonStringify(updatedMetadata), + }), + }) + .then(() => + this.props.dispatch( + dashboardInfoChanged({ + metadata: updatedMetadata, + }), + ), + ) + .catch(e => console.error(e)); + } + } + handleClickTab(tabIndex) { const { component } = this.props; const { children: tabIds } = component; + // the charts need to render first + setTimeout(() => { + this.updateLabelColorsMap(); + }, 500); + if (tabIndex !== this.state.tabIndex) { const pathToTabIndex = getDirectPathToTabIndex(component, tabIndex); const targetTabId = pathToTabIndex[pathToTabIndex.length - 1]; diff --git a/superset-frontend/src/dashboard/containers/DashboardComponent.jsx b/superset-frontend/src/dashboard/containers/DashboardComponent.jsx index 68478adb07..f600463ee9 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 c01dc76f1e..d1bb7c2ac4 100644 --- a/superset-frontend/src/dashboard/containers/DashboardPage.tsx +++ b/superset-frontend/src/dashboard/containers/DashboardPage.tsx @@ -188,6 +188,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }: PageProps) => { useEffect(() => { const sharedLabelColor = getSharedLabelColor(); sharedLabelColor.source = SharedLabelColorSource.Dashboard; + return () => { // clean up label color const categoricalNamespace = CategoricalColorNamespace.getNamespace( diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx index dba78af035..d7c1a804a9 100644 --- a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx +++ b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx @@ -24,6 +24,7 @@ import { Tooltip } from 'src/components/Tooltip'; import { CategoricalColorNamespace, css, + getSharedLabelColor, logging, SupersetClient, t, @@ -95,6 +96,16 @@ export const ExploreChartHeader = ({ const { dashboards } = metadata || {}; const dashboard = dashboardId && dashboards && dashboards.find(d => d.id === dashboardId); + const categoricalNamespace = CategoricalColorNamespace.getNamespace(); + const sharedLabelColor = getSharedLabelColor(); + + if (!dashboard) { + // clean up color namespace and shared color maps + // to avoid colors spill outside of dashboard context + categoricalNamespace.resetColors(); + sharedLabelColor.clear(); + return; + } if (dashboard) { try { @@ -106,21 +117,19 @@ 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 dashboardMetadata = JSON.parse(result.json_metadata); + const sharedLabelColors = dashboardMetadata.shared_label_colors || {}; + const customLabelColors = dashboardMetadata.label_colors || {}; const mergedLabelColors = { ...sharedLabelColors, ...customLabelColors, }; - const categoricalNamespace = CategoricalColorNamespace.getNamespace(); - Object.keys(mergedLabelColors).forEach(label => { categoricalNamespace.setColor( label, mergedLabelColors[label], - metadata.color_scheme, + dashboardMetadata.color_scheme, ); }); } catch (error) { @@ -130,7 +139,7 @@ export const ExploreChartHeader = ({ }; useEffect(() => { - if (dashboardId) updateCategoricalNamespace(); + updateCategoricalNamespace(); }, []); const openPropertiesModal = () => { diff --git a/tests/integration_tests/superset_test_config.py b/tests/integration_tests/superset_test_config.py index b0461e79fc..977eb1c6b5 100644 --- a/tests/integration_tests/superset_test_config.py +++ b/tests/integration_tests/superset_test_config.py @@ -69,6 +69,7 @@ FEATURE_FLAGS = { "SHARE_QUERIES_VIA_KV_STORE": True, "ENABLE_TEMPLATE_PROCESSING": True, "ALERT_REPORTS": True, + "AVOID_COLORS_COLLISION": True, "DRILL_TO_DETAIL": True, "DRILL_BY": True, "HORIZONTAL_FILTER_BAR": True,
