diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx index 7df56939218..ed61890e710 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx @@ -361,6 +361,18 @@ export const StyledChartContainer = styled.div<{ font-size: ${theme.fontSizeSM}px; } + /* + * AG Grid 34+ adds the row-entrance-animation class 'ag-opacity-zero' + * (opacity: 0) to a newly inserted row and removes it on the next frame to + * fade the row in. Under AG Grid 36 that class is never removed from the + * pinned bottom row, so the "Show summary" totals row renders fully (correct + * values in the DOM) but stays permanently transparent. Force pinned rows + * opaque so the summary row is visible. + */ + .ag-row-pinned { + opacity: 1 !important; + } + .ag-spanned-row { font-size: ${theme.fontSizeSM}px; font-weight: ${theme.fontWeightStrong}; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts index 5389418d967..e2d3934212f 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts @@ -628,7 +628,10 @@ const transformProps = ( percentDifferenceNum, col.colorScheme || comparisonColorScheme, ); - item[col.column] = { + // Key by the metric column key (not the raw rule column) so the + // renderer's `col.metricName` lookup resolves it, identical to + // the comparison-color path below. + item[origCol.key] = { mainArrow: arrow, arrowColor, backgroundColor, @@ -717,9 +720,32 @@ const transformProps = ( const passedData = isUsingTimeComparison ? comparisonData || [] : data; const passedColumns = isUsingTimeComparison ? comparisonColumns : columns; - const basicColorFormatters = + // Increase/decrease formatters from the "Comparison color" toggle, keyed by + // metric column key. + const comparisonColorFormatters = comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns); + // Custom conditional-formatting rules using the Green (increase) / Red + // (decrease) color scheme on a time-comparison table. These were computed but + // never consumed by the AG Grid renderer, so the colors/arrows never showed + // (the classic plugin-chart-table does consume them). Route them through the + // same increase/decrease path, keyed by metric column key (see above). + const basicColorColumnFormatters = getBasicColorFormatterForColumn( + baseQuery?.data, + columns, + conditionalFormatting, + ); + + // Merge both per-row into a single map so the existing row-attached formatter + // drives the renderer for either source. + const basicColorFormatters = + comparisonColorFormatters || basicColorColumnFormatters + ? (baseQuery?.data ?? []).map((_row, index) => ({ + ...(comparisonColorFormatters || [])[index], + ...(basicColorColumnFormatters || [])[index], + })) + : comparisonColorFormatters; + // Attach each row's basic (increase/decrease) color formatter to the row data // object so it travels with the row through AG Grid client-side sorting. // basicColorFormatters is built in the original query order and was previously @@ -737,14 +763,20 @@ const transformProps = ( }); }); } - const columnColorFormatters = - getColorFormatters(conditionalFormatting, passedData, theme) ?? []; - const basicColorColumnFormatters = getBasicColorFormatterForColumn( - baseQuery?.data, - columns, - conditionalFormatting, - ); + // Green/Red custom rules are rendered via the increase/decrease path above, so + // exclude them here: getColorFormatters treats the scheme name as a hex color + // and would emit an invalid `'FF'` background otherwise. + const columnColorFormatters = + getColorFormatters( + (conditionalFormatting || []).filter( + (config: ConditionalFormattingConfig) => + config.colorScheme !== ColorSchemeEnum.Green && + config.colorScheme !== ColorSchemeEnum.Red, + ), + passedData, + theme, + ) ?? []; const hasPageLength = isPositiveNumber(pageLength); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts index 665909c6f7b..7e3964c6c2a 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts @@ -85,11 +85,19 @@ const getCellStyle = (params: CellStyleParams) => { col?.metricName && node?.rowPinned !== 'bottom' ) { - backgroundColor = getRowBasicColorFormatter( + const basicBackgroundColor = getRowBasicColorFormatter( node, rowIndex, basicColorFormatters, )?.[col.metricName]?.backgroundColor; + // Only override when this column actually has an increase/decrease + // formatter. Green/Red conditional-format rules only target some metrics, + // so a column carrying only a standard conditional-format rule would + // otherwise have its background clobbered with `undefined` when Green/Red + // rules exist on other columns. + if (basicBackgroundColor) { + backgroundColor = basicBackgroundColor; + } } const textAlign = diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts index 7b870c5ab72..c0b9ba2240f 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts @@ -264,3 +264,53 @@ test('uses description from column even when verboseMap renames the column', () expect(columnMeta!.label).toBe('Custom Label'); expect(columnMeta!.description).toBe('Original column description'); }); + +test('excludes Green/Red color-scheme rules from columnColorFormatters', () => { + // Green/Red rules are rendered via the increase/decrease path, so they must + // not reach getColorFormatters, which would treat the scheme name as a hex + // color and emit an invalid `'GreenFF'` background. + const props = createMockChartProps({ + rawFormData: { + viz_type: 'table', + datasource: '1__table', + query_mode: QueryMode.Aggregate, + metrics: ['metric_a', 'metric_b'], + percent_metrics: [], + column_config: {}, + table_timestamp_format: '', + granularity_sqla: 'day', + time_range: 'No filter', + conditional_formatting: [ + { + column: 'metric_a', + operator: '>', + targetValue: 0, + colorScheme: 'Green', + }, + { + column: 'metric_b', + operator: '>', + targetValue: 0, + colorScheme: '#FF0000', + }, + ], + } as unknown as TableChartProps['rawFormData'], + queriesData: [ + { + data: [{ metric_a: 10, metric_b: 20 }], + colnames: ['metric_a', 'metric_b'], + coltypes: [GenericDataType.Numeric, GenericDataType.Numeric], + rowcount: 1, + applied_filters: [], + rejected_filters: [], + }, + ] as unknown as TableChartProps['queriesData'], + }); + + const result = transformProps(props); + const formattedColumns = result.columnColorFormatters.map(f => f.column); + // The standard (#FF0000) rule is kept... + expect(formattedColumns).toContain('metric_b'); + // ...but the Green rule is excluded. + expect(formattedColumns).not.toContain('metric_a'); +}); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts new file mode 100644 index 00000000000..b420ddc5f0f --- /dev/null +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts @@ -0,0 +1,108 @@ +/** + * 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 getCellStyle from '../../src/utils/getCellStyle'; + +// A standard conditional-formatting rule that paints metric_a red. +const standardCfFormatter = { + column: 'metric_a', + getColorFromValue: () => '#ff0000', + objectFormatting: undefined, + toTextColor: false, +}; + +function buildParams(overrides: Record = {}) { + return { + value: 100, + valueFormatted: '100', + colDef: { field: 'metric_a' }, + rowIndex: 0, + node: { rowPinned: undefined, data: {} }, + col: { + key: 'metric_a', + metricName: 'metric_a', + isNumeric: true, + config: {}, + }, + cellSurfaceColor: '#ffffff', + hoverCellSurfaceColor: '#eeeeee', + hasColumnColorFormatters: false, + columnColorFormatters: [], + hasBasicColorFormatters: false, + basicColorFormatters: undefined, + ...overrides, + } as unknown as Parameters[0]; +} + +test('applies a standard conditional-format background', () => { + const style = getCellStyle( + buildParams({ + hasColumnColorFormatters: true, + columnColorFormatters: [standardCfFormatter], + }), + ); + expect(style.backgroundColor).toBe('#ff0000'); +}); + +test('preserves the standard CF background when the column has no increase/decrease formatter', () => { + // hasBasicColorFormatters is enabled (e.g. a Green/Red rule exists on another + // column), but this column has no basic formatter entry. The standard + // conditional-format background must not be clobbered with undefined. + const style = getCellStyle( + buildParams({ + hasColumnColorFormatters: true, + columnColorFormatters: [standardCfFormatter], + hasBasicColorFormatters: true, + basicColorFormatters: [{}], // row 0 has no formatter for metric_a + }), + ); + expect(style.backgroundColor).toBe('#ff0000'); +}); + +test('applies the increase/decrease background when the column has one', () => { + const style = getCellStyle( + buildParams({ + hasColumnColorFormatters: true, + columnColorFormatters: [standardCfFormatter], + hasBasicColorFormatters: true, + basicColorFormatters: [ + { + metric_a: { + backgroundColor: '#00ff00', + mainArrow: '↑', + arrowColor: 'green', + }, + }, + ], + }), + ); + expect(style.backgroundColor).toBe('#00ff00'); +}); + +test('does not apply basic formatting to the pinned summary row', () => { + const style = getCellStyle( + buildParams({ + node: { rowPinned: 'bottom', data: {} }, + hasBasicColorFormatters: true, + basicColorFormatters: [ + { metric_a: { backgroundColor: '#00ff00', mainArrow: '↑' } }, + ], + }), + ); + expect(style.backgroundColor).toBe(''); +});