EnxDev commented on PR #41031: URL: https://github.com/apache/superset/pull/41031#issuecomment-4901054300
## EnxDev's Review Agent โ apache/superset#41031 ยท HEAD 8b41124 lgtm โ re-review of new commits; every prior finding is addressed. One stale title clause to fix before merge. _Supersedes my [prior review](https://github.com/apache/superset/pull/41031#issuecomment-4809966822) (HEAD f55eca2)._ **Prior findings โ resolved at head:** - ๐ด Modal `overflow: hidden` clipping in-modal popups โ the entire modal change (`Modal.tsx`, `Select/styles.tsx`, `RunQueryActionButton`) is gone from the diff. No longer clips. - ๐ก Uncovered mapping logic โ extracted to `buildResultsGridThemeOverrides.ts` with 9 tests covering each token, the `false`/non-number guards, and the all-set case. - ๐ก `Record<string, any>` โ now typed `GridThemeOverrides`. - ๐ก Stale description (`selectOptionActiveOutline`) โ body reconciled; only the 5 `resultsGrid*` tokens remain. **Verified the tokens are live, not dead.** Each key the builder emits is spread into `themeQuartz.withParams()` in `ThemedAgGridReact`. Confirmed against AG Grid v34 docs that all seven are real theme parameters โ `headerFontSize`, `headerFontWeight`, `headerHeight`, `rowHeight`, `borderRadius`, `wrapperBorderRadius`, `oddRowBackgroundColor`. No token set โ builder returns `undefined` โ `withParams` is skipped, so defaults are provably unchanged. (Bito's "invalid ag-grid theme parameter" flag is a false positive.) ### ๐ก Should-fix - **PR title** โ still reads *"and fix modal border-radius clipping"*, but that fix was removed (correctly โ it globally clipped in-modal popups). Drop the clause; otherwise the squashed `feat:` changelog line advertises a modal fix that isn't in the PR. If the modal clipping is still worth fixing, do it in its own PR without the global `overflow: hidden`. - **Docs** โ the 5 new deployer-facing `resultsGrid*` tokens aren't documented. Add them to the theme-token docs, next to the neighboring `buttonSecondary*` tokens in `types.ts`. ### ๐ต Nits - **`TabbedSqlEditors/index.tsx:47-83`** โ `!important` โ `&&` relies on antd v5's `:where()` low specificity to keep winning on the active-tab border/box-shadow. Screenshot shows the default is unchanged; worth a quick confirm in dark mode where a deployer stylesheet could now out-specify these. - **`GridTable/index.tsx:143-153`** โ with a row-height token set, `rowHeight`/`headerHeight` land via both `gridOptions` and `withParams`. Harmless (same value), just redundant. ### ๐ Praise - Clean response to review: a pure, typed util with 9 focused regression tests, and the buggy modal change dropped rather than patched over. <!-- enxdev-review-agent:8b41124 --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD 8b41124._ -- 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]
