fix(sql-lab): fix transparent background in ag-grid native menus - #43368
Draft
sadpandajoe wants to merge 2 commits into
Draft
fix(sql-lab): fix transparent background in ag-grid native menus#43368sadpandajoe wants to merge 2 commits into
sadpandajoe wants to merge 2 commits into
Conversation
…tooltips ThemedAgGridReact set backgroundColor to transparent so the surrounding app shows through the grid body, but never set the separate params (chromeBackgroundColor, menuBackgroundColor, sideBarBackgroundColor, tooltipBackgroundColor, modalOverlayBackgroundColor) that control the background of ag-grid's own context/column menus, side bar, tooltips and loading/no-rows overlays. Those inherited the transparency too, making native right-click and column menus unreadable wherever they render (e.g. the SQL Lab results grid). Set these params to theme.colorBgElevated and enable menuBorder for better readability against the surrounding surface, matching the resolved default background used elsewhere in the app.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43368 +/- ##
===========================================
- Coverage 78.82% 66.93% -11.89%
===========================================
Files 2876 2876
Lines 164459 163920 -539
Branches 37956 37888 -68
===========================================
- Hits 129634 109725 -19909
- Misses 32378 52044 +19666
+ Partials 2447 2151 -296
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
ThemedAgGridReact(the shared wrapper aroundAgGridReactused 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 setbackgroundColor: '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,
overlayBackgroundColorwas 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 compiledag-grid-communityruntime bundle (dist/ag-grid-community.js) and its shipped type declarations (core-css.d.ts), not just documentation. The real param ismodalOverlayBackgroundColor, whose own default ({ ref: 'backgroundColor', mix: 0.66 }) explains why it fell back to the transparentbackgroundColorvalue — ag-grid silently ignores unrecognized keys rather than erroring, so the typo produced no build or runtime warning.This PR:
chromeBackgroundColor,menuBackgroundColor,sideBarBackgroundColor, andtooltipBackgroundColorparams.overlayBackgroundColor→ the realmodalOverlayBackgroundColorparam.menuBorder: truefor 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.themeOverridesstill 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 ofThemedAgGridReact. 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
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 indist/types/src/theming/core/core-css.d.ts.npx jest packages/superset-ui-core/src/components/ThemedAgGridReact/applies non-transparent backgrounds to native menus, tooltips and overlaysassertschromeBackgroundColor,menuBackgroundColor,menuBorder,sideBarBackgroundColor,tooltipBackgroundColor, andmodalOverlayBackgroundColorall resolve to the theme'scolorBgElevated.npx jest src/components/GridTable/GridTable.test.tsx src/SqlLab/components/ResultSet/ResultSet.test.tsx.ADDITIONAL INFORMATION