codeant-ai-for-open-source[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2955775990
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -997,8 +1047,14 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
? basicColorColumnFormatters[row.index][column.key]?.mainArrow
: '';
}
+ const rowSurfaceColor =
+ row.index % 2 === 0 ? theme.colorBgLayout : theme.colorBgBase;
+ const resolvedTextColor = getTextColorForBackground(
+ { backgroundColor, color },
+ rowSurfaceColor,
+ );
const StyledCell = styled.td`
- color: ${color ? `${color}FF` : theme.colorText};
+ color: ${resolvedTextColor || theme.colorText};
Review Comment:
**Suggestion:** This always injects an inline text color, which overrides
the `.dt-is-null` class color and breaks null-value styling. Apply an inline
color only when a formatter actually resolved one, so null cells can still use
the existing muted class-based color. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Null table cells lose muted visual distinction.
- ⚠️ Classic table conditional-format readability consistency regresses.
```
</details>
```suggestion
color: ${resolvedTextColor || undefined};
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Table chart (plugin entrypoint
`superset-frontend/plugins/plugin-chart-table/src/index.ts:75` loads
`./TableChart`).
2. Use query results containing nulls (test fixture proves real shape at
`.../test/testData.ts:124` with `__timestamp: null`;
`transformProps.ts:222,702,755`
passes query data directly to `TableChart`).
3. In cell rendering, null values are explicitly tagged `dt-is-null` at
`TableChart.tsx:1150-1153`, and the stylesheet defines muted null color at
`Styles.tsx:114-116` (`colorTextTertiary`).
4. The same cell also gets `StyledCell` color `resolvedTextColor ||
theme.colorText` at
`TableChart.tsx:1057`; when no formatter applies
(`getTextColorForBackground` returns
undefined at `TableChart.tsx:124-125`), this forces normal text color and
overrides
intended null-muted styling.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 1057:1057
**Comment:**
*Logic Error: This always injects an inline text color, which overrides
the `.dt-is-null` class color and breaks null-value styling. Apply an inline
color only when a formatter actually resolved one, so null cells can still use
the existing muted class-based color.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=9031b2093a7aa6a513e62bdff82a9f43ffe6e23662dc35f2c3050fc2848ca206&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=9031b2093a7aa6a513e62bdff82a9f43ffe6e23662dc35f2c3050fc2848ca206&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:
##########
@@ -280,15 +282,20 @@ export const useColDefs = ({
headerName: getHeaderLabel(col),
valueFormatter: p => valueFormatter(p, col),
valueGetter: p => valueGetter(p, col),
- cellStyle: p =>
- getCellStyle({
+ cellStyle: p => {
+ const cellStyleParams = {
...p,
hasColumnColorFormatters,
columnColorFormatters,
hasBasicColorFormatters,
basicColorFormatters,
col,
- }),
+ cellBackgroundColor: theme.colorBgBase,
Review Comment:
**Suggestion:** Adaptive text contrast is being computed against
`theme.colorBgBase` for every row, but AG Grid renders striped odd rows with a
different background. For semi-transparent conditional background colors, this
produces the wrong composite color on odd rows and can pick an unreadable text
color. Use row parity to pass the actual row surface color. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ AG Grid odd rows may get weaker contrast.
- ⚠️ Semi-transparent formatter backgrounds render inconsistent text color.
- ❌ Conditional-format readability differs from actual row surface.
```
</details>
```suggestion
cellBackgroundColor:
(p.rowIndex ?? 0) % 2 === 1
? theme.colorFillQuaternary
: theme.colorBgBase,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render an AG Grid table chart, which builds column defs through
`useColDefs()` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:227-246`.
2. Ensure conditional formatting includes a semi-transparent background
color (supported
by formatter outputs, e.g. `rgba(...)`, and consumed in `getCellStyle()` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:65-77`).
3. AG Grid uses zebra rows with odd-row surface `theme.colorFillQuaternary`
via
`oddRowBackgroundColor` in
`superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx:80-100`.
4. Current code passes only `theme.colorBgBase` as `cellBackgroundColor` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts:293`,
so
`getTextColorForBackground()` composites against the wrong surface
(`superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:22-30`)
on odd rows and can choose less readable text.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
**Line:** 293:293
**Comment:**
*Logic Error: Adaptive text contrast is being computed against
`theme.colorBgBase` for every row, but AG Grid renders striped odd rows with a
different background. For semi-transparent conditional background colors, this
produces the wrong composite color on odd rows and can pick an unreadable text
color. Use row parity to pass the actual row surface color.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a6a016c81c185442da849d31a7beb296eb18fd695b17254c13d5a2a7c8cd892b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=a6a016c81c185442da849d31a7beb296eb18fd695b17254c13d5a2a7c8cd892b&reaction=dislike'>👎</a>
--
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]
