From 208a929de9520261856d49d3f9a24461feca8358 Mon Sep 17 00:00:00 2001 From: Diego Pucci Date: Thu, 17 Oct 2024 20:24:03 +0300 Subject: [PATCH] fix(Dashboard): Keep track of charts origin color schemes --- .../cypress/e2e/dashboard/editmode.test.ts | 4 +- .../src/color/LabelsColorMapSingleton.ts | 50 +++++++- .../color/LabelsColorMapSingleton.test.ts | 41 ++++++ .../src/dashboard/actions/dashboardState.js | 120 ++++++++++-------- .../DashboardBuilder/DashboardContainer.tsx | 16 ++- superset-frontend/src/utils/colorScheme.ts | 10 +- 6 files changed, 170 insertions(+), 71 deletions(-) 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 314e0f6048d47..cec0b6aca5a27 100644 --- a/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts @@ -507,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(108, 69, 146)'); + .should('have.css', 'fill', 'rgb(0, 128, 246)'); // open main tab and nested tab openTab(0, 0); @@ -518,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(108, 69, 146)'); + .should('have.css', 'fill', 'rgb(0, 128, 246)'); }); it('should apply the color scheme across main tabs', () => { diff --git a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts index 59d3f8cc5de79..c44304f14238c 100644 --- a/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts +++ b/superset-frontend/packages/superset-ui-core/src/color/LabelsColorMapSingleton.ts @@ -17,7 +17,9 @@ * under the License. */ +import { getCategoricalSchemeRegistry } from '.'; import { makeSingleton } from '../utils'; +import CategoricalColorNamespace from './CategoricalColorNamespace'; export enum LabelsColorMapSource { Dashboard, @@ -38,7 +40,16 @@ export class LabelsColorMap { this.source = LabelsColorMapSource.Dashboard; } - updateColorMap(categoricalNamespace: any, colorScheme?: string) { + /** + * Wipes out the color map and updates it with the new color scheme. + * + * @param categoricalNamespace - the namespace to use for color mapping + * @param colorScheme - color scheme + */ + updateColorMap( + categoricalNamespace: CategoricalColorNamespace, + colorScheme?: string, + ) { const newColorMap = new Map(); this.colorMap.clear(); this.chartsLabelsMap.forEach((chartConfig, sliceId) => { @@ -58,6 +69,16 @@ export class LabelsColorMap { return this.colorMap; } + /** + * + * Called individually by each plugin via getColor fn. + * + * @param label - the label name + * @param color - the color + * @param sliceId - the chart id + * @param colorScheme - the color scheme + * + */ addSlice( label: string, color: string, @@ -75,12 +96,34 @@ export class LabelsColorMap { labels.push(label); this.chartsLabelsMap.set(sliceId, { labels, - scheme: colorScheme, + scheme: this.chartsLabelsMap.get(sliceId)?.scheme || colorScheme, }); } this.colorMap.set(label, color); } + /** + * Keeps track of original colors of charts in their formData. + * + * @param sliceId - chart id + * @param colorScheme - color scheme + */ + setSliceOriginColorScheme(sliceId: number, colorScheme?: string) { + const categoricalSchemes = getCategoricalSchemeRegistry(); + const fallbackColorScheme = + categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors'; + + this.chartsLabelsMap.set(sliceId, { + labels: this.chartsLabelsMap.get(sliceId)?.labels || [], + scheme: colorScheme || fallbackColorScheme, + }); + } + + /** + * Remove a slice from the color map. + * + * @param sliceId - the chart + */ removeSlice(sliceId: number) { if (this.source !== LabelsColorMapSource.Dashboard) return; @@ -96,6 +139,9 @@ export class LabelsColorMap { this.colorMap = newColorMap; } + /** + * Clear the shared labels color map. + */ clear() { this.chartsLabelsMap.clear(); this.colorMap.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 index b93a416e7ffaa..eaed2e4a671be 100644 --- a/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/color/LabelsColorMapSingleton.test.ts @@ -60,6 +60,29 @@ describe('LabelsColorMap', () => { expect(getLabelsColorMap()).toBeInstanceOf(LabelsColorMap); }); + describe('.setSliceOriginColorScheme(sliceId, scheme)', () => { + it('should set slice color scheme', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.setSliceOriginColorScheme(1, 'testColors'); + expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual( + 'testColors', + ); + }); + it('should fallback to default color scheme if not provided', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.setSliceOriginColorScheme(1); + expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual( + 'SUPERSET_DEFAULT', + ); + }); + it('should re-use existing labels when setting color scheme', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.addSlice('a', 'red', 1); + labelsColorMap.setSliceOriginColorScheme(1, 'testColors'); + expect(labelsColorMap.chartsLabelsMap.get(1)?.labels).toEqual(['a']); + }); + }); + describe('.addSlice(value, color, sliceId)', () => { it('should add to sliceLabelColorMap when first adding label', () => { const labelsColorMap = getLabelsColorMap(); @@ -98,6 +121,24 @@ describe('LabelsColorMap', () => { labelsColorMap.addSlice('a', 'red', 1); expect(Object.fromEntries(labelsColorMap.chartsLabelsMap)).toEqual({}); }); + + it('should use set slice color scheme via setSliceOriginColorScheme', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.setSliceOriginColorScheme(1, 'testColors2'); + labelsColorMap.addSlice('a', 'red', 1); + expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual( + 'testColors2', + ); + }); + + it('should use given color scheme when not set via setSliceOriginColorScheme', () => { + const labelsColorMap = getLabelsColorMap(); + labelsColorMap.setSliceOriginColorScheme(2, 'testColors2'); + labelsColorMap.addSlice('a', 'red', 1, 'testColors'); + expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual( + 'testColors', + ); + }); }); describe('.remove(sliceId)', () => { diff --git a/superset-frontend/src/dashboard/actions/dashboardState.js b/superset-frontend/src/dashboard/actions/dashboardState.js index 10fcf518cbd6b..1b70fe73f2101 100644 --- a/superset-frontend/src/dashboard/actions/dashboardState.js +++ b/superset-frontend/src/dashboard/actions/dashboardState.js @@ -595,6 +595,7 @@ export function addSliceToDashboard(id) { dispatch(fetchDatasourceMetadata(form_data.datasource)), ]).then(() => { dispatch(addSlice(selectedSlice)); + getLabelsColorMap().setSliceOriginColorScheme(id, form_data.color_scheme); }); }; } @@ -686,62 +687,73 @@ const updateDashboardMetadata = async (id, metadata, dispatch) => { 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); - } +export const updateDashboardLabelsColor = + charts => async (dispatch, getState) => { + const { + dashboardInfo: { id, metadata }, + } = getState(); + const categoricalSchemes = getCategoricalSchemeRegistry(); + const sharedColorMapInstance = getLabelsColorMap(); + 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; + + // track original color scheme for each chart + charts.forEach(chart => + sharedColorMapInstance.setSliceOriginColorScheme( + chart.id, + chart.form_data?.color_scheme, + ), + ); - // stored labels color map and applied might differ - // new data may appear in the map (data changes) - // or new slices may appear while changing tabs - const isMapSynced = isLabelsColorMapSynced(metadata); - if (!isMapSynced) { - // re-apply a fresh labels color map - applyColors(updatedMetadata, false, true); - // pull and store the just applied labels color map - updatedMetadata.shared_label_colors = getLabelsColorMapEntries(); - } + // Color scheme does not exist anymore, fallback to default + if (colorScheme && !colorSchemeRegistry) { + updatedScheme = fallbackScheme; + updatedMetadata.color_scheme = updatedScheme; + updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme); - // the stored color domain registry and fresh might differ at this point - const freshColorSchemeDomain = getColorSchemeDomain(colorScheme); - const isRegistrySynced = - colorSchemeDomain.toString() === freshColorSchemeDomain.toString(); + dispatch(setColorScheme(updatedScheme)); + // must re-apply colors from fresh labels color map + applyColors(updatedMetadata, true); + } - if (colorScheme && !isRegistrySynced) { - updatedMetadata.color_scheme_domain = freshColorSchemeDomain; - } + // stored labels color map and applied might differ + // new data may appear in the map (data changes) + // or new slices may appear while changing tabs + // requires merging the stored and applied maps + const isMapSynced = isLabelsColorMapSynced(metadata); + if (!isMapSynced) { + // re-apply a fresh labels color map + applyColors(updatedMetadata, false, true); + // pull and store the just applied labels color map + updatedMetadata.shared_label_colors = getLabelsColorMapEntries(); + } - if ( - (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) || - !isMapSynced - ) { - await updateDashboardMetadata(id, updatedMetadata, dispatch); + // 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); } - } catch (error) { - console.error('Failed to update dashboard color settings:', error); - } -}; + }; diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx index 7c27a9f1f7888..53be05eecd794 100644 --- a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx +++ b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx @@ -31,6 +31,7 @@ import { pick } from 'lodash'; import Tabs from 'src/components/Tabs'; import DashboardGrid from 'src/dashboard/containers/DashboardGrid'; import { + Chart, DashboardInfo, DashboardLayout, LayoutItem, @@ -85,8 +86,13 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { const directPathToChild = useSelector( state => state.dashboardState.directPathToChild, ); - const chartIds = useSelector(state => - Object.values(state.charts).map(chart => chart.id), + const charts = useSelector(state => + Object.values(state.charts), + ); + + const chartIds: number[] = useMemo( + () => charts.map(chart => chart.id), + [charts], ); const prevTabIndexRef = useRef(); @@ -144,9 +150,11 @@ const DashboardContainer: FC = ({ topLevelTabs }) => { // verify freshness of color map on tab change // and when loading for first time setTimeout(() => { - dispatch(updateDashboardLabelsColor()); + if (charts.length) { + dispatch(updateDashboardLabelsColor(charts)); + } }, 500); - }, [directPathToChild, dispatch]); + }, [directPathToChild, dispatch, charts]); useEffect(() => { const labelsColorMap = getLabelsColorMap(); diff --git a/superset-frontend/src/utils/colorScheme.ts b/superset-frontend/src/utils/colorScheme.ts index a3da1427c74c7..03cd03d266f76 100644 --- a/superset-frontend/src/utils/colorScheme.ts +++ b/superset-frontend/src/utils/colorScheme.ts @@ -115,18 +115,10 @@ export const applyColors = ( const colorNameSpace = getColorNamespace(metadata?.color_namespace); const categoricalNamespace = CategoricalColorNamespace.getNamespace(colorNameSpace); - let colorScheme = metadata?.color_scheme; + const colorScheme = metadata?.color_scheme; const customLabelColors = metadata?.label_colors || {}; - // when scheme unset, update only custom label colors const labelsColorMap = metadata?.shared_label_colors || {}; - if (!colorScheme) { - // a fallback color scheme must be set to generate shared label colors - const categoricalSchemes = getCategoricalSchemeRegistry(); - colorScheme = - categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors'; - } - if (fresh) { // reset forced colors (custom labels + labels color map) categoricalNamespace.resetColors();