This is an automated email from the ASF dual-hosted git repository. rusackas pushed a commit to branch feat/ag-grid-zebra-striping in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6dae021eb15dbe534893fc298418de42805c14b3 Author: Evan Rusackas <[email protected]> AuthorDate: Wed Sep 9 12:09:08 2026 -0700 feat(ag-grid-table): add zebra striping control Table V2's shared ag-grid wrapper (ThemedAgGridReact) already renders a subtle odd-row background unconditionally for every AG Grid instance in Superset, including SQL Lab's results grid. It's just much subtler than Table V1's overt Bootstrap-style striping, easy to miss side by side. Add a zebra_striping checkbox (default false) so Table V2 charts can turn that subtle background on or off per chart, reusing the same shared theme token rather than introducing a new color, and scoped to this plugin via ThemedAgGridReact's existing themeOverrides prop so SQL Lab's grid and other consumers are unaffected. A migrated v1 chart gets zebra_striping forced on, since v1's striping was never a per-chart toggle to begin with. --- .../src/AgGridTable/index.tsx | 19 +++++++ .../src/AgGridTableChart.tsx | 3 ++ .../src/controlPanel.tsx | 14 +++++ .../src/transformProps.ts | 2 + .../plugin-chart-ag-grid-table/src/types.ts | 2 + .../src/utils/useColDefs.ts | 9 +++- .../test/controlPanel.test.ts | 12 +++++ .../test/transformProps.test.ts | 25 +++++++++ .../test/utils/useColDefs.test.ts | 62 ++++++++++++++++++++++ .../migrations/shared/migrate_viz/processors.py | 8 +++ .../unit_tests/migrations/viz/table_v1_v2_test.py | 18 +++++++ 11 files changed, 173 insertions(+), 1 deletion(-) diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx index a395f44c52b..4015532a7c1 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx @@ -45,6 +45,7 @@ import { SelectionChangedEvent, } from '@superset-ui/core/components/ThemedAgGridReact'; import { t } from '@apache-superset/core/translation'; +import { useTheme } from '@apache-superset/core/theme'; import { AgGridChartState, DataRecordValue, @@ -118,6 +119,7 @@ export interface AgGridTableProps { gridRef?: RefObject<AgGridReact>; chartState?: AgGridChartState; onClientViewChange?: (snapshot: ClientViewSnapshot) => void; + zebraStriping: boolean; } ModuleRegistry.registerModules([AllCommunityModule, ClientSideRowModelModule]); @@ -159,6 +161,7 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo( metricColumns = [], chartState, onClientViewChange, + zebraStriping, }) => { const gridRef = useRef<AgGridReact>(null); const inputRef = useRef<HTMLInputElement>(null); @@ -168,6 +171,21 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo( const hasCapturedInitialGridStateRef = useRef(false); const filterOperationVersionRef = useRef(0); + const theme = useTheme(); + // ThemedAgGridReact defaults every AG Grid instance (including SQL Lab's + // results grid) to a subtle striped background via this same token, so + // an explicit override is needed in both directions here rather than + // just turning it on: "off" has to opt out of that shared default, and + // "on" reuses the same token instead of inventing a chart-specific color. + const themeOverrides = useMemo( + () => ({ + oddRowBackgroundColor: zebraStriping + ? theme.colorFillQuaternary + : 'transparent', + }), + [zebraStriping, theme.colorFillQuaternary], + ); + const searchId = `search-${id}`; const initialFilterModel = getInitialFilterModel( @@ -668,6 +686,7 @@ const AgGridDataTable: FunctionComponent<AgGridTableProps> = memo( <ThemedAgGridReact ref={gridRef} onGridReady={onGridReady} + themeOverrides={themeOverrides} className="ag-container" rowData={rowData} headerHeight={36} diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx index bbc5baf1e8f..e9d11770271 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx @@ -99,6 +99,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( metricSqlExpressions, rawSummaryColumns, showNumberedColumn, + zebraStriping, onContextMenu, formData, } = props; @@ -362,6 +363,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( conditionalFormatting: formData?.conditional_formatting, comparisonColorEnabled: formData?.comparison_color_enabled, comparisonColorScheme: formData?.comparison_color_scheme, + zebraStriping, }); const isActiveFilterValue = useCallback( @@ -730,6 +732,7 @@ export default function TableChart<D extends DataRecord = DataRecord>( onColumnStateChange={handleColumnStateChange} chartState={chartState} onClientViewChange={handleClientViewChange} + zebraStriping={!!zebraStriping} /> </StyledChartContainer> ); diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx index 4d5a60d933d..4711a58009c 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx @@ -597,6 +597,20 @@ const config: ControlPanelConfig = { }, }, ], + [ + { + name: 'zebra_striping', + config: { + type: 'CheckboxControl', + label: t('Zebra striping'), + renderTrigger: true, + default: false, + description: t( + 'Alternate the row background color to make rows easier to scan.', + ), + }, + }, + ], [ { name: 'column_config', 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 82fcbd3242a..fde606dae12 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 @@ -590,6 +590,7 @@ const transformProps = ( show_numbered_column: showNumberedColumn = false, allow_rearrange_columns: allowRearrangeColumns = true, allow_render_html: allowRenderHtml = true, + zebra_striping: zebraStriping = false, } = formData; // Calculate time comparison settings early since they're used in multiple places @@ -950,6 +951,7 @@ const transformProps = ( chartState, onChartStateChange, showNumberedColumn, + zebraStriping, onContextMenu, }; }; diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts index 90f617fdbae..85bc3485ee5 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts @@ -84,6 +84,7 @@ export type TableChartFormData = QueryFormData & { allow_rearrange_columns?: boolean; allow_render_html?: boolean; show_numbered_column?: boolean; + zebra_striping?: boolean; }; export interface TableChartProps extends ChartProps { @@ -136,6 +137,7 @@ export interface AgGridTableChartTransformedProps< onChartStateChange?: (chartState: JsonObject) => void; chartState?: AgGridChartState; showNumberedColumn: boolean; + zebraStriping: boolean; onContextMenu?: ( clientX: number, clientY: number, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts index 05136466e61..c795f95ed63 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts @@ -84,6 +84,7 @@ type UseColDefsProps = { conditionalFormatting?: ConditionalFormattingConfig[]; comparisonColorEnabled?: boolean; comparisonColorScheme?: string; + zebraStriping?: boolean; }; function getValueRange( @@ -253,6 +254,7 @@ export const useColDefs = ({ conditionalFormatting, comparisonColorEnabled, comparisonColorScheme, + zebraStriping, }: UseColDefsProps) => { const theme = useTheme(); // transformProps.ts computes these fresh on every call (no memoization), @@ -332,8 +334,12 @@ export const useColDefs = ({ valueFormatter: (p: ValueFormatterParams) => valueFormatter(p, col), valueGetter: (p: ValueGetterParams) => valueGetter(p, col), cellStyle: (p: CellClassParams) => { + // Mirrors the oddRowBackgroundColor override AgGridTable passes to + // ThemedAgGridReact for this same zebraStriping flag, so the color + // used for text-contrast here always matches the row's actual + // rendered background. const cellSurfaceColor = - p.node?.rowPinned != null + p.node?.rowPinned != null || !zebraStriping ? theme.colorBgBase : p.rowIndex % 2 === 0 ? theme.colorBgBase @@ -476,6 +482,7 @@ export const useColDefs = ({ allowRenderHtml, serverPagination, alignPositiveNegative, + zebraStriping, theme.colorBgBase, theme.colorFillSecondary, theme.colorFillQuaternary, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts index 90368c8a6aa..12af40cd3d8 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts @@ -226,3 +226,15 @@ test('allow_render_html defaults to true, matching v1, and has no visibility gat expect(control.config.renderTrigger).toBe(true); expect(control.config.visibility).toBeUndefined(); }); + +test('zebra_striping defaults to false and has no visibility gate', () => { + // v1 has no equivalent control at all (its striping is unconditional), so + // there's no "matching v1" default here -- new v2 charts default to v2's + // own subtle look, and the migration processor is what materializes True + // for charts migrated from v1. + const control = findControl(config, 'zebra_striping'); + expect(control.config.type).toBe('CheckboxControl'); + expect(control.config.default).toBe(false); + expect(control.config.renderTrigger).toBe(true); + expect(control.config.visibility).toBeUndefined(); +}); 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 923bb41e6a5..61c12b3bc06 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 @@ -432,6 +432,31 @@ test('allowRenderHtml is false when allow_render_html is explicitly false', () = expect(result.allowRenderHtml).toBe(false); }); +test('zebraStriping defaults to false when zebra_striping is unset', () => { + const props = createMockChartProps(); + const result = transformProps(props); + expect(result.zebraStriping).toBe(false); +}); + +test('zebraStriping is true when zebra_striping is explicitly true', () => { + const props = createMockChartProps({ + rawFormData: { + viz_type: 'table', + datasource: '1__table', + query_mode: QueryMode.Aggregate, + metrics: [], + percent_metrics: [], + column_config: {}, + table_timestamp_format: '', + granularity_sqla: 'day', + time_range: 'No filter', + zebra_striping: true, + } as unknown as TableChartProps['rawFormData'], + }); + const result = transformProps(props); + expect(result.zebraStriping).toBe(true); +}); + test('retains saved percentage rules with automatic bounds when server pagination is enabled', () => { const props = createMockChartProps({ rawFormData: { diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts index 428bf4b8991..d6025e328d5 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts @@ -470,6 +470,7 @@ test('cellStyle uses striped odd-row surface for adaptive contrast', () => { ...defaultProps, columns: [numericCol], data: [{ count: 42 }, { count: 43 }], + zebraStriping: true, columnColorFormatters: [ { column: 'count', @@ -516,6 +517,67 @@ test('cellStyle uses striped odd-row surface for adaptive contrast', () => { }); }); +test('cellStyle uses a flat surface for contrast when zebra striping is disabled', () => { + // zebraStriping defaults to false/undefined -- AgGridTable's + // oddRowBackgroundColor override matches this by rendering a flat + // background in that case, so the contrast color computed here must not + // alternate either, or it'd pick a color meant for a stripe that isn't + // actually there. + const numericCol = makeColumn({ + key: 'count', + label: 'Count', + dataType: GenericDataType.Numeric, + isNumeric: true, + isMetric: true, + }); + const backgroundColor = 'rgba(0, 0, 0, 0.4)'; + + const { result } = renderHook( + () => + useColDefs({ + ...defaultProps, + columns: [numericCol], + data: [{ count: 42 }, { count: 43 }], + columnColorFormatters: [ + { + column: 'count', + objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR, + getColorFromValue: (value: unknown) => + typeof value === 'number' ? backgroundColor : undefined, + }, + ], + }), + { + wrapper: makeThemeWrapper({ + ...supersetTheme, + colorBgBase: '#ffffff', + colorFillQuaternary: '#000000', + }), + }, + ); + + const cellStyle = getCellStyleFunction(result.current[0].cellStyle); + const expectedTextColor = getExpectedTextColor( + { backgroundColor }, + '#ffffff', + ); + + expect( + getCellStyleResult(cellStyle, { + rowIndex: 0, + }), + ).toMatchObject({ + '--ag-cell-value-color': expectedTextColor, + }); + expect( + getCellStyleResult(cellStyle, { + rowIndex: 1, + }), + ).toMatchObject({ + '--ag-cell-value-color': expectedTextColor, + }); +}); + test('cellStyle exposes hover-specific adaptive contrast for formatted cells', () => { const numericCol = makeColumn({ key: 'count', diff --git a/superset/migrations/shared/migrate_viz/processors.py b/superset/migrations/shared/migrate_viz/processors.py index 2f48fd8c47c..f854dd203cb 100644 --- a/superset/migrations/shared/migrate_viz/processors.py +++ b/superset/migrations/shared/migrate_viz/processors.py @@ -799,6 +799,14 @@ class MigrateTableChart(MigrateViz): if "allow_rearrange_columns" not in self.data: self.data["allow_rearrange_columns"] = False + # v1's TableChart always renders with Bootstrap-style zebra + # striping ("table-striped") -- there's no control for it, it's + # unconditional. v2's zebra_striping control defaults new charts to + # False (matching v2's own subtle-by-default look), so a migrated + # chart needs this materialized explicitly to keep its original + # striped appearance rather than silently losing it. + self.data["zebra_striping"] = True + def _build_aggregate_mode_query( self, base_query_object: dict[str, Any], time_offsets: list[Any] ) -> tuple[list[Any], list[Any], Any, list[Any]]: diff --git a/tests/unit_tests/migrations/viz/table_v1_v2_test.py b/tests/unit_tests/migrations/viz/table_v1_v2_test.py index a1627954dbc..8c3c1b3b91c 100644 --- a/tests/unit_tests/migrations/viz/table_v1_v2_test.py +++ b/tests/unit_tests/migrations/viz/table_v1_v2_test.py @@ -44,6 +44,9 @@ SOURCE_FORM_DATA: dict[str, Any] = { } TARGET_FORM_DATA: dict[str, Any] = { + # zebra_striping is intentionally absent from SOURCE_FORM_DATA: v1 has + # no control for it (always striped), so the migration materializes it + # unconditionally rather than carrying over a source key. "datasource": "1__table", "any_other_key": "untouched", "viz_type": "ag-grid-table", @@ -62,6 +65,7 @@ TARGET_FORM_DATA: dict[str, Any] = { "color_pn": True, "allow_rearrange_columns": True, "allow_render_html": True, + "zebra_striping": True, "form_data_bak": SOURCE_FORM_DATA, } @@ -242,6 +246,20 @@ def test_migration_defaults_omitted_allow_rearrange_columns_to_false() -> None: migrate_and_assert(MigrateTableChart, source, target) +def test_migration_always_enables_zebra_striping() -> None: + """v1's TableChart always renders striped rows -- there's no control + for it, so no source key exists to carry over. v2's zebra_striping + control defaults new charts to False, so the migration must + unconditionally set it True for a migrated chart to keep its original + striped appearance.""" + target: dict[str, Any] = { + **{k: v for k, v in TARGET_FORM_DATA.items() if k != "form_data_bak"}, + "zebra_striping": True, + "form_data_bak": SOURCE_FORM_DATA, + } + migrate_and_assert(MigrateTableChart, SOURCE_FORM_DATA, target) + + @pytest.mark.parametrize( "auto_currency_form_data", [
