joaopedroab commented on PR #38705: URL: https://github.com/apache/superset/pull/38705#issuecomment-4100747310
I took a second pass specifically on the AG Grid comments. The stale recycled-cell issue from the earlier review is already fixed on the branch. In `getCellStyle`, when no formatter matches, the returned style now explicitly resets: - `color` to `''` - `--ag-cell-value-color` to `''` - `--ag-cell-value-hover-color` to `''` That matters because AG Grid recycles DOM cells. If those inline properties are omitted instead of reset, a reused cell can keep the previous row's text color. The current branch clears those values, and the AG Grid regression test covers that exact case. Current AG Grid flow is: - `useColDefs` computes the effective base/striped/hover surface from the theme - `getCellStyle` resolves explicit text color first, otherwise derives readable contrast from the formatted background against that surface - the style object stores the resolved text colors in inline CSS custom properties - the stylesheet reads those inline properties for normal and hover states I also re-checked the remaining AG Grid bot comments: - The comment claiming the custom properties are undefined is a false positive. They are intentionally defined inline by `cellStyle` per cell, then consumed by `.ag-cell` and `.ag-row-hover .ag-cell`. - The `CELL_BAR` concern is already handled. `CELL_BAR` is excluded from text/background color resolution, and there is test coverage for that path. Validation I reran locally: - `npx jest --runInBand --runTestsByPath plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts` - `npx jest --runInBand --runTestsByPath packages/superset-ui-chart-controls/test/utils/getColorFormatters.test.ts` - `npx jest --runInBand --runTestsByPath plugins/plugin-chart-table/test/TableChart.test.tsx` - `npx jest --runInBand --runTestsByPath plugins/plugin-chart-pivot-table/test/react-pivottable/tableRenders.test.tsx` - `pre-commit run --files superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/useColDefs.test.ts` I pushed one small follow-up commit only for reviewer clarity: - explicit return types on the shared contrast helpers - renaming the AG Grid test so it describes the real behavior: resetting inline variables when no formatter matches -- 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]
