bito-code-review[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2959570731
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -72,9 +90,28 @@ const getCellStyle = (params: CellStyleParams) => {
const textAlign =
col?.config?.horizontalAlign || (col?.isNumeric ? 'right' : 'left');
+ const resolvedTextColor = getTextColorForBackground(
+ { backgroundColor, color },
+ cellSurfaceColor,
+ );
+ const hoverResolvedTextColor = getTextColorForBackground(
+ { backgroundColor, color },
+ hoverCellSurfaceColor,
+ );
return {
backgroundColor: backgroundColor || '',
+ ...(resolvedTextColor
+ ? {
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': resolvedTextColor,
+ }
+ : {}),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid AG Grid cellStyle property</b></div>
<div id="fix">
The cellStyle sets color to 'var(--ag-cell-value-color)', but AG Grid
doesn't support this custom property, so text color won't apply. Use the
resolved value directly for proper contrast.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
...(resolvedTextColor
? {
color: resolvedTextColor,
}
: {}),
````
</div>
</details>
</div>
<small><i>Code Review Run #2f9811</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -280,15 +282,30 @@ export const useColDefs = ({
headerName: getHeaderLabel(col),
valueFormatter: p => valueFormatter(p, col),
valueGetter: p => valueGetter(p, col),
- cellStyle: p =>
- getCellStyle({
+ cellStyle: p => {
+ const cellSurfaceColor =
+ p.node?.rowPinned != null
+ ? theme.colorBgBase
+ : p.rowIndex % 2 === 0
+ ? theme.colorBgBase
+ : theme.colorFillQuaternary;
+ const hoverCellSurfaceColor =
+ p.node?.rowPinned != null
+ ? cellSurfaceColor
+ : theme.colorFillSecondary;
+ const cellStyleParams = {
...p,
hasColumnColorFormatters,
columnColorFormatters,
hasBasicColorFormatters,
basicColorFormatters,
col,
- }),
+ cellSurfaceColor,
+ hoverCellSurfaceColor,
+ } as Parameters<typeof getCellStyle>[0];
+
+ return getCellStyle(cellStyleParams);
+ },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect text color calculation due to unset
background</b></div>
<div id="fix">
The change passes cellSurfaceColor to getCellStyle for text color
calculation, but getCellStyle does not set the cell backgroundColor to
cellSurfaceColor. This causes text color to be calculated for a background that
isn't actually set, leading to potential poor contrast or incorrect visibility.
For proper zebra striping and hover effects, the background should match the
surface color used for text color computation.
</div>
</div>
<small><i>Code Review Run #2f9811</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts:
##########
@@ -121,15 +212,619 @@ test('temporal columns use custom TextCellRenderer', ()
=> {
dataType: GenericDataType.Temporal,
});
- const { result } = renderHook(() =>
- useColDefs({
- ...defaultProps,
- columns: [temporalCol],
- data: [{ created_at: '2024-01-01' }],
- }),
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [temporalCol],
+ data: [{ created_at: '2024-01-01' }],
+ }),
+ { wrapper: defaultThemeWrapper },
);
const colDef = result.current[0];
expect(colDef.cellRenderer).toBeInstanceOf(Function);
expect(colDef.cellDataType).toBe('date');
});
+
+test('cellStyle derives readable text color from dark background formatting',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(255, 255, 255)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle keeps explicit text color over adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle treats legacy toTextColor formatters as text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle uses caller-provided surface color for adaptive contrast', ()
=> {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result: lightResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ }),
+ },
+ );
+
+ const { result: darkResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#000000',
+ }),
+ },
+ );
+
+ const lightCellStyle =
getCellStyleFunction(lightResult.current[0].cellStyle);
+ const darkCellStyle = getCellStyleFunction(darkResult.current[0].cellStyle);
+
+ expect(getCellStyleResult(lightCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(getCellStyleResult(darkCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+ expect(getCellStyleResult(lightCellStyle)).not.toEqual(
+ getCellStyleResult(darkCellStyle),
+ );
+});
+
+test('cellStyle uses striped odd-row surface for adaptive contrast', () => {
+ 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);
+
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 0,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 1,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle exposes hover-specific adaptive contrast for formatted cells',
() => {
+ 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 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillSecondary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ '--ag-cell-value-hover-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle falls back to cellTextColor when no formatter matches', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: () => undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorPrimaryText: '#123456',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ const cellStyleResult = getCellStyleResult(cellStyle) as {
+ backgroundColor: string;
+ color?: string;
+ textAlign: string;
+ };
+ expect(cellStyleResult).toMatchObject({
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test expectations for cell style
fallback</b></div>
<div id="fix">
The test 'cellStyle falls back to cellTextColor when no formatter matches'
expects the style object to include `color: ''`, `'--ag-cell-value-color': ''`,
and `'--ag-cell-value-hover-color': ''`, but the `getCellStyle` function only
includes these properties when `resolvedTextColor` is defined. When no
background color or text color is set by formatters, these properties are
omitted. Additionally, the test sets `colorPrimaryText` in the theme but this
property is not used by the cell style logic. This will cause the test to fail
as the expected properties are absent from the returned object.
</div>
</div>
<small><i>Code Review Run #2f9811</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts:
##########
@@ -121,15 +212,619 @@ test('temporal columns use custom TextCellRenderer', ()
=> {
dataType: GenericDataType.Temporal,
});
- const { result } = renderHook(() =>
- useColDefs({
- ...defaultProps,
- columns: [temporalCol],
- data: [{ created_at: '2024-01-01' }],
- }),
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [temporalCol],
+ data: [{ created_at: '2024-01-01' }],
+ }),
+ { wrapper: defaultThemeWrapper },
);
const colDef = result.current[0];
expect(colDef.cellRenderer).toBeInstanceOf(Function);
expect(colDef.cellDataType).toBe('date');
});
+
+test('cellStyle derives readable text color from dark background formatting',
() => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(255, 255, 255)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle keeps explicit text color over adaptive contrast', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(
+ cellStyle({
+ value: 42,
+ colDef: { field: 'count' },
+ rowIndex: 0,
+ node: {},
+ } as never),
+ ).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle treats legacy toTextColor formatters as text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#111111' : undefined,
+ },
+ {
+ column: 'count',
+ toTextColor: true,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? '#ace1c40d' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const colDef = result.current[0];
+ const cellStyle = getCellStyleFunction(colDef.cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '#111111',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'rgb(172, 225, 196)',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle uses caller-provided surface color for adaptive contrast', ()
=> {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+ const backgroundColor = 'rgba(0, 0, 0, 0.4)';
+
+ const { result: lightResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ }),
+ },
+ );
+
+ const { result: darkResult } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#000000',
+ }),
+ },
+ );
+
+ const lightCellStyle =
getCellStyleFunction(lightResult.current[0].cellStyle);
+ const darkCellStyle = getCellStyleFunction(darkResult.current[0].cellStyle);
+
+ expect(getCellStyleResult(lightCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(getCellStyleResult(darkCellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+ expect(getCellStyleResult(lightCellStyle)).not.toEqual(
+ getCellStyleResult(darkCellStyle),
+ );
+});
+
+test('cellStyle uses striped odd-row surface for adaptive contrast', () => {
+ 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);
+
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 0,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ });
+ expect(
+ getCellStyleResult(cellStyle, {
+ rowIndex: 1,
+ }),
+ ).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle exposes hover-specific adaptive contrast for formatted cells',
() => {
+ 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 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? backgroundColor : undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorBgBase: '#ffffff',
+ colorFillSecondary: '#000000',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor,
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': getExpectedTextColor(
+ { backgroundColor },
+ '#ffffff',
+ ),
+ '--ag-cell-value-hover-color': getExpectedTextColor(
+ { backgroundColor },
+ '#000000',
+ ),
+ });
+});
+
+test('cellStyle falls back to cellTextColor when no formatter matches', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.BACKGROUND_COLOR,
+ getColorFromValue: () => undefined,
+ },
+ ],
+ }),
+ {
+ wrapper: makeThemeWrapper({
+ ...supersetTheme,
+ colorPrimaryText: '#123456',
+ }),
+ },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ const cellStyleResult = getCellStyleResult(cellStyle) as {
+ backgroundColor: string;
+ color?: string;
+ textAlign: string;
+ };
+ expect(cellStyleResult).toMatchObject({
+ backgroundColor: '',
+ color: '',
+ '--ag-cell-value-color': '',
+ '--ag-cell-value-hover-color': '',
+ textAlign: 'right',
+ });
+});
+
+test('cellStyle preserves invalid explicit text color', () => {
+ const numericCol = makeColumn({
+ key: 'count',
+ label: 'Count',
+ dataType: GenericDataType.Numeric,
+ isNumeric: true,
+ isMetric: true,
+ });
+
+ const { result } = renderHook(
+ () =>
+ useColDefs({
+ ...defaultProps,
+ columns: [numericCol],
+ data: [{ count: 42 }],
+ columnColorFormatters: [
+ {
+ column: 'count',
+ objectFormatting: ObjectFormattingEnum.TEXT_COLOR,
+ getColorFromValue: (value: unknown) =>
+ value === 42 ? 'not-a-color' : undefined,
+ },
+ ],
+ }),
+ { wrapper: defaultThemeWrapper },
+ );
+
+ const cellStyle = getCellStyleFunction(result.current[0].cellStyle);
+ expect(getCellStyleResult(cellStyle)).toMatchObject({
+ backgroundColor: '',
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': 'not-a-color',
+ '--ag-cell-value-hover-color': 'not-a-color',
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Invalid color values not validated</b></div>
<div id="fix">
The test documents that invalid color strings like 'not-a-color' are
preserved in CSS variables, which could result in invalid CSS being applied to
DOM elements. Consider validating colors and falling back to undefined for
invalid values.
</div>
</div>
<small><i>Code Review Run #2f9811</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts:
##########
@@ -107,6 +112,55 @@ test('getColorFunction LESS_THAN', () => {
expect(colorFunction(50)).toEqual('#FF0000FF');
});
+test('getReadableTextColor returns white for dark backgrounds', () => {
+ expect(getReadableTextColor('#111111', '#ffffff')).toBe('rgb(255, 255,
255)');
+});
+
+test('getReadableTextColor returns black for light backgrounds', () => {
+ expect(getReadableTextColor('#f5f5f5', '#ffffff')).toBe('rgb(0, 0, 0)');
+});
+
+test('getReadableTextColor blends alpha over the provided surface', () => {
+ expect(getReadableTextColor('rgba(0, 0, 0, 0.6)', '#ffffff')).toBe(
+ 'rgb(255, 255, 255)',
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test expectation for color contrast</b></div>
<div id="fix">
The test expects white text for a blended gray background, but
getReadableTextColor correctly chooses black for better AA contrast (9:1 vs
2.3:1). This would cause the test to fail against the proper implementation.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
expect(getReadableTextColor('rgba(0, 0, 0, 0.6)', '#ffffff')).toBe(
'rgb(0, 0, 0)',
);
````
</div>
</details>
</div>
<small><i>Code Review Run #2f9811</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
