This is an automated email from the ASF dual-hosted git repository. michaelsmolina pushed a commit to branch 5.0 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7ed236170dff96c7819df93a2232fa9f7d3de99b Author: Fardin Mustaque <[email protected]> AuthorDate: Tue Mar 25 17:52:15 2025 +0530 fix: Time Comparison Feature Reverts Metric Labels to Metric Keys in Table Charts (#32665) Co-authored-by: Fardin Mustaque <[email protected]> (cherry picked from commit 7d77dc4fd2001bfa1a752fb9a2da0cdc7349f886) --- .../plugins/plugin-chart-table/src/TableChart.tsx | 8 ++- .../plugin-chart-table/src/transformProps.ts | 5 ++ .../plugins/plugin-chart-table/src/types.ts | 2 + .../plugin-chart-table/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 77905ea562..95121a317d 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<D extends DataRecord = DataRecord>( // 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 <th> for columns before this header for (let i = currentColumnIndex; i < startPosition; i += 1) { @@ -620,7 +622,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( // Add the current header <th> headers.push( <th key={`header-${key}`} colSpan={colSpan} style={{ borderBottom: 0 }}> - {key} + {originalLabel} <span css={css` float: right; @@ -975,7 +977,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( ), Footer: totals ? ( i === 0 ? ( - <th> + <th key={`footer-summary-${i}`}> <div css={css` display: flex; @@ -997,7 +999,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( </div> </th> ) : ( - <td style={sharedStyle}> + <td key={`footer-total-${i}`} style={sharedStyle}> <strong>{formatColumnValue(column, totals[key])[1]}</strong> </td> ) diff --git a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts index 48871e4ea4..d62d9cb92c 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 1ec3cbe29d..62a666a88e 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 b21a657b81..b74e1ffccf 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( + <ThemeProvider theme={supersetTheme}> + <TableChart {...transformProps(testData.comparison)} sticky={false} /> + </ThemeProvider>, + ); + + 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', () => {
