From 7d77dc4fd2001bfa1a752fb9a2da0cdc7349f886 Mon Sep 17 00:00:00 2001 From: Fardin Mustaque <105560328+fardin-developer@users.noreply.github.com> Date: Tue, 25 Mar 2025 17:52:15 +0530 Subject: [PATCH] fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts (#32665) Co-authored-by: Fardin Mustaque --- .../plugin-chart-table/src/TableChart.tsx | 8 +- .../plugin-chart-table/src/transformProps.ts | 5 ++ .../plugins/plugin-chart-table/src/types.ts | 2 + .../test/TableChart.test.tsx | 80 +++++++++++++++++++ 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 77905ea562e..95121a317d4 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -605,6 +605,8 @@ export default function TableChart( // Calculate the number of placeholder columns needed before the current header const startPosition = value[0]; const colSpan = value.length; + // Retrieve the originalLabel from the first column in this group + const originalLabel = columnsMeta[value[0]]?.originalLabel || key; // Add placeholder for columns before this header for (let i = currentColumnIndex; i < startPosition; i += 1) { @@ -620,7 +622,7 @@ export default function TableChart( // Add the current header headers.push( - {key} + {originalLabel} ( ), Footer: totals ? ( i === 0 ? ( - +
(
) : ( - + {formatColumnValue(column, totals[key])[1]} ) diff --git a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts index 48871e4ea41..d62d9cb92c9 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts @@ -347,6 +347,7 @@ const processComparisonColumns = ( } = props; const savedFormat = columnFormats?.[col.key]; const savedCurrency = currencyFormats?.[col.key]; + const originalLabel = col.label; if ( (col.isMetric || col.isPercentMetric) && !col.key.includes(comparisonSuffix) && @@ -355,6 +356,7 @@ const processComparisonColumns = ( return [ { ...col, + originalLabel, label: t('Main'), key: `${t('Main')} ${col.key}`, config: getComparisonColConfig(t('Main'), col.key, columnConfig), @@ -368,6 +370,7 @@ const processComparisonColumns = ( }, { ...col, + originalLabel, label: `#`, key: `# ${col.key}`, config: getComparisonColConfig(`#`, col.key, columnConfig), @@ -381,6 +384,7 @@ const processComparisonColumns = ( }, { ...col, + originalLabel, label: `△`, key: `△ ${col.key}`, config: getComparisonColConfig(`△`, col.key, columnConfig), @@ -394,6 +398,7 @@ const processComparisonColumns = ( }, { ...col, + originalLabel, label: `%`, key: `% ${col.key}`, config: getComparisonColConfig(`%`, col.key, columnConfig), diff --git a/superset-frontend/plugins/plugin-chart-table/src/types.ts b/superset-frontend/plugins/plugin-chart-table/src/types.ts index 1ec3cbe29d7..62a666a88e7 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/types.ts @@ -56,6 +56,8 @@ export interface DataColumnMeta { key: string; // `label` is verbose column name used for rendering label: string; + // `originalLabel` preserves the original label when time comparison transforms the labels + originalLabel?: string; dataType: GenericDataType; formatter?: | TimeFormatter diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index b21a657b815..b74e1ffccf4 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -175,6 +175,75 @@ describe('plugin-chart-table', () => { ?.formatter?.(0.123456); expect(formattedPercentMetric).toBe('0.123'); }); + + it('should set originalLabel for comparison columns when time_compare and comparison_type are set', () => { + const transformedProps = transformProps(testData.comparison); + + // Check if comparison columns are processed + const comparisonColumns = transformedProps.columns.filter( + col => + col.label === 'Main' || + col.label === '#' || + col.label === '△' || + col.label === '%', + ); + + expect(comparisonColumns.length).toBeGreaterThan(0); + expect(comparisonColumns.some(col => col.label === 'Main')).toBe(true); + expect(comparisonColumns.some(col => col.label === '#')).toBe(true); + expect(comparisonColumns.some(col => col.label === '△')).toBe(true); + expect(comparisonColumns.some(col => col.label === '%')).toBe(true); + + // Verify originalLabel for metric_1 comparison columns + const mainMetric1 = transformedProps.columns.find( + col => col.key === 'Main metric_1', + ); + expect(mainMetric1).toBeDefined(); + expect(mainMetric1?.originalLabel).toBe('metric_1'); + + const hashMetric1 = transformedProps.columns.find( + col => col.key === '# metric_1', + ); + expect(hashMetric1).toBeDefined(); + expect(hashMetric1?.originalLabel).toBe('metric_1'); + + const deltaMetric1 = transformedProps.columns.find( + col => col.key === '△ metric_1', + ); + expect(deltaMetric1).toBeDefined(); + expect(deltaMetric1?.originalLabel).toBe('metric_1'); + + const percentMetric1 = transformedProps.columns.find( + col => col.key === '% metric_1', + ); + expect(percentMetric1).toBeDefined(); + expect(percentMetric1?.originalLabel).toBe('metric_1'); + + // Verify originalLabel for metric_2 comparison columns + const mainMetric2 = transformedProps.columns.find( + col => col.key === 'Main metric_2', + ); + expect(mainMetric2).toBeDefined(); + expect(mainMetric2?.originalLabel).toBe('metric_2'); + + const hashMetric2 = transformedProps.columns.find( + col => col.key === '# metric_2', + ); + expect(hashMetric2).toBeDefined(); + expect(hashMetric2?.originalLabel).toBe('metric_2'); + + const deltaMetric2 = transformedProps.columns.find( + col => col.key === '△ metric_2', + ); + expect(deltaMetric2).toBeDefined(); + expect(deltaMetric2?.originalLabel).toBe('metric_2'); + + const percentMetric2 = transformedProps.columns.find( + col => col.key === '% metric_2', + ); + expect(percentMetric2).toBeDefined(); + expect(percentMetric2?.originalLabel).toBe('metric_2'); + }); }); describe('TableChart', () => { @@ -400,6 +469,17 @@ describe('plugin-chart-table', () => { ); expect(getComputedStyle(screen.getByText('N/A')).background).toBe(''); }); + it('should display originalLabel in grouped headers', () => { + render( + + + , + ); + + const groupHeaders = screen.getAllByRole('columnheader'); + expect(groupHeaders[0]).toHaveTextContent('metric_1'); + expect(groupHeaders[1]).toHaveTextContent('metric_2'); + }); }); it('render cell bars properly, and only when it is toggled on in both regular and percent metrics', () => {