sadpandajoe opened a new pull request, #43368:
URL: https://github.com/apache/superset/pull/43368
### SUMMARY
`ThemedAgGridReact` (the shared wrapper around `AgGridReact` used across the
app, including SQL Lab's results grid) builds its ag-grid theme via ag-grid
v34+'s JS theming API (`themeQuartz.withParams(...)`). Its base params
intentionally set `backgroundColor: 'transparent'` so the surrounding app
surface shows through the grid body, but the params that control the background
of ag-grid's own **native** popups — context/column menus, side bar, tooltips,
and loading/no-rows overlays — were either missing entirely or set to a key
that isn't a real ag-grid theming param.
Specifically, `overlayBackgroundColor` was used in the params object, but
that key does not exist in ag-grid's theming API at all — I verified this
directly against the compiled `ag-grid-community` runtime bundle
(`dist/ag-grid-community.js`) and its shipped type declarations
(`core-css.d.ts`), not just documentation. The real param is
`modalOverlayBackgroundColor`, whose own default (`{ ref: 'backgroundColor',
mix: 0.66 }`) explains why it fell back to the transparent `backgroundColor`
value — ag-grid silently ignores unrecognized keys rather than erroring, so the
typo produced no build or runtime warning.
This PR:
- Adds the previously-missing `chromeBackgroundColor`,
`menuBackgroundColor`, `sideBarBackgroundColor`, and `tooltipBackgroundColor`
params.
- Fixes `overlayBackgroundColor` → the real `modalOverlayBackgroundColor`
param.
- Adds `menuBorder: true` for readability against the surrounding surface.
All five resolve to `theme.colorBgElevated`, matching the app's existing
elevated-surface color rather than introducing a new one. `themeOverrides`
still merges on top of these per-instance as before, so any caller already
setting these explicitly is unaffected.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No screenshot/GIF is included. The affected UI surface — ag-grid's native
context/column menu — only renders when an ag-grid Enterprise module (e.g.
`ContextMenuModule`/`ColumnMenuModule`) is registered on top of
`ThemedAgGridReact`. Vanilla OSS Superset does not register these modules
anywhere, so this defect cannot currently be visually reproduced by running OSS
Superset alone; it only manifests for downstream consumers that layer licensed
ag-grid Enterprise modules onto this shared component. The fix is instead
validated at the source/bundle level (see TESTING INSTRUCTIONS) and via a unit
test that was confirmed RED before the fix, then GREEN after — including a
manual revert-and-recheck to confirm the assertion actually catches the invalid
key rather than passing tautologically.
### TESTING INSTRUCTIONS
1. Verify the invalid key directly against the installed package:
- `grep -o "overlayBackgroundColor"
node_modules/ag-grid-community/dist/ag-grid-community.js` → no matches (key
does not exist).
- `grep -o "modalOverlayBackgroundColor"
node_modules/ag-grid-community/dist/ag-grid-community.js` → present, including
in `dist/types/src/theming/core/core-css.d.ts`.
2. Run the updated unit test suite:
- `npx jest packages/superset-ui-core/src/components/ThemedAgGridReact/`
- The new test `applies non-transparent backgrounds to native menus,
tooltips and overlays` asserts `chromeBackgroundColor`, `menuBackgroundColor`,
`menuBorder`, `sideBarBackgroundColor`, `tooltipBackgroundColor`, and
`modalOverlayBackgroundColor` all resolve to the theme's `colorBgElevated`.
3. Confirm no regression in the grid's primary SQL Lab consumers: `npx jest
src/components/GridTable/GridTable.test.tsx
src/SqlLab/components/ResultSet/ResultSet.test.tsx`.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
