codeant-ai-for-open-source[bot] commented on PR #36991: URL: https://github.com/apache/superset/pull/36991#issuecomment-3726509005
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36991/files#diff-3a1680cdfe4669587cb7013f1f21b93a0ea2d049aa110e2e34bd16aa733c7607R122-R129'><strong>Accessibility / Contrast</strong></a><br>The hover/active background was changed to `theme.colorBgSpotlight`. This color may not provide sufficient contrast with cell text in all themes (light/dark, high-contrast). Verify contrast ratio and keyboard focus visibility for these states across themes and ensure WCAG requirements are met.<br> - [ ] <a href='https://github.com/apache/superset/pull/36991/files#diff-335dc5438784500da588f0597bccb57da0efd761532dafee2531d4a589feb5bfR290-R298'><strong>Accessibility / contrast</strong></a><br>The new hover color `theme.colorBgSpotlight` should be checked for sufficient contrast against the row/cell content color when used as hover/active background to maintain accessibility (WCAG) standards. Ensure the chosen grayscale hover meets contrast targets for readable text/icons.<br> - [ ] <a href='https://github.com/apache/superset/pull/36991/files#diff-335dc5438784500da588f0597bccb57da0efd761532dafee2531d4a589feb5bfR294-R298'><strong>Background override risk</strong></a><br>The new rules set `background-color` on hover while `.dt-is-active-filter` uses `background` (shorthand) for the base state. Depending on how other styles (e.g., gradients or AG Grid injected styles) are applied, `background-color` may not fully override a previously-set `background` shorthand or image. Confirm hover styling reliably overrides any AG or inherited background declarations.<br> - [ ] <a href='https://github.com/apache/superset/pull/36991/files#diff-3a1680cdfe4669587cb7013f1f21b93a0ea2d049aa110e2e34bd16aa733c7607R122-R129'><strong>Scope / Consistency</strong></a><br>PR description mentions reverting hover/active colors for AG and non-AG grids, but this change only touches the table plugin CSS. Confirm the same token is applied wherever grid hover/active styles are defined (AG Grid themes or other table components) to ensure consistent behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36991/files#diff-335dc5438784500da588f0597bccb57da0efd761532dafee2531d4a589feb5bfR290-R298'><strong>Inconsistent hover colors</strong></a><br>The PR changes `.dt-is-filter` and `.dt-is-active-filter` hover color to `theme.colorBgSpotlight` but other interactive elements (for example `.menu-item &:hover` in `MenuContainer`) still use `theme.colorPrimaryBgHover`. This can lead to inconsistent hover styling across the table UI and may negate the stated goal of reverting hover states to grayscale. Verify all related hover states are intentional and consistent.<br> </td></tr> </table> -- 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]
