fix(plugin-chart-ag-grid-table): render Show summary totals row on ag-grid 36#42115
Conversation
…-grid 36 The "Show summary" totals row is invisible on the AG Grid Interactive Table after the ag-grid 36 upgrade (#41882). The pinned bottom row is built and populated correctly (the DOM contains the row with the correct aggregated values), but ag-grid 36 leaves its row-entrance animation class `ag-opacity-zero` (opacity: 0) permanently applied to the pinned bottom row. Normally ag-grid removes that class on the next frame to fade the row in; under v36 it is never removed for pinned rows, so the summary row renders fully but stays transparent. Force pinned rows opaque in the chart's styled container, overriding the stuck ag-grid animation class, so the summary row is visible again.
dosubot
Bot
added
change:frontend
Code Review Agent Run #d11175Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42115 +/- ##
=======================================
Coverage 65.13% 65.13%
=======================================
Files 2753 2753
Lines 154573 154584 +11
Branches 35453 35463 +10
=======================================
+ Hits 100674 100689 +15
+ Misses 51985 51981 -4
Partials 1914 1914
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:
|
…ing on time-comparison tables Custom conditional-formatting rules using the Green (increase) / Red (decrease) color scheme did not render on the AG Grid Interactive Table. transformProps built `basicColorColumnFormatters` but nothing in the renderer consumed it (the classic plugin-chart-table does consume it). Other color schemes (alert/gradient/error/success) work because they flow through `columnColorFormatters`, which is consumed. Route the Green/Red rules through the existing increase/decrease formatter path: key them by the metric column key and merge them into the row-attached `basicColorFormatters` map that getCellStyle (background) and NumericCellRenderer (arrow) already consume, so they render like the classic table and stay correct through client-side sorting (#105973). Also exclude Green/Red rules from getColorFormatters to avoid an invalid '<scheme>FF' background from a non-hex color-scheme name.
1cd3287 to
4d2ee4e
Compare
…acks
oxlint (no-useless-fallback-in-spread) flags the `|| {}` fallbacks when
spreading a possibly-undefined per-row formatter: `{ ...(x || {}) }` is
equivalent to `{ ...x }` since spreading undefined/null in an object
literal is a no-op. Apply the autofix so pre-commit passes.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current merge logic creates sparse objects where missing keys for metrics without Green/Red rules result in Here is a concise fix for the merge logic in const basicColorFormatters = (baseQuery?.data ?? []).map((_row, index) => {
const comparison = (comparisonColorFormatters || [])[index] || {};
const basic = (basicColorColumnFormatters || [])[index] || {};
return { ...comparison, ...basic };
});This approach ensures that the resulting object is a clean merge of the two formatters without introducing superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts |
… with empty basic formatter When Green/Red conditional-format rules exist, hasBasicColorFormatters is enabled for the whole table, but those rules only target some metric columns. getCellStyle unconditionally overwrote the cell background with the basic (increase/decrease) formatter's backgroundColor, which is undefined for columns without a Green/Red rule, wiping a standard conditional-format background on those columns. Only apply the basic background when the column actually has an increase/decrease formatter, preserving standard conditional formatting on untouched columns. Comparison-color mode is unaffected (it builds a formatter for every metric column).
EnxDev
left a comment
EnxDev
left a comment
There was a problem hiding this comment.
@amaannawab923 thanks for handling this, can we add some tests?
…round guard Adds unit tests for the two conditional-formatting fixes: - getCellStyle preserves a standard conditional-format background when the column has no increase/decrease formatter (Green/Red rules only target some columns), applies the basic background when present, and skips the pinned summary row. - transformProps excludes Green/Red color-scheme rules from columnColorFormatters while keeping standard rules.
|
Added test @EnxDev
11 tests green locally across |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
Two rendering fixes for the AG Grid "Interactive Table" (Table v2), both involving the ag-grid 36 upgrade / found alongside it. The data pipelines are correct in both cases — the failures are purely in the render layer.
1. "Show summary" totals row not rendering (ag-grid 36 regression)
The totals row is invisible after the ag-grid 36 upgrade (#41882). The pinned bottom row is built and populated correctly — the DOM contains the row with the correct aggregated values — but ag-grid 36 leaves its row-entrance animation class
ag-opacity-zero(opacity: 0) permanently applied to the pinned bottom row. Normally ag-grid removes that class on the next frame to fade the row in; under v36 it is never removed for pinned rows, so the summary row renders fully but stays transparent. Fixed by forcing pinned rows opaque in the chart's styled container.Before
After
2. Green/Red conditional formatting not rendering on time-comparison tables
Custom conditional-formatting rules using the Green (increase) / Red (decrease) color scheme never rendered on the AG Grid table.
transformPropsbuildsbasicColorColumnFormattersbut nothing in the renderer consumed it (the classicplugin-chart-tabledoes consume it viabasicColorColumnFormatters[row.index][column.key]). Other schemes (alert/gradient/error/success) work because they flow throughcolumnColorFormatters, which is consumed. Fixed by routing the Green/Red rules through the existing increase/decrease formatter path — keyed by the metric column key and merged into the row-attachedbasicColorFormattersmap thatgetCellStyle(background) andNumericCellRenderer(arrow) already consume, so they render like the classic table and stay correct through client-side sorting (#105973) — and by excluding Green/Red rules fromgetColorFormatters(which otherwise emits an invalid'GreenFF'background from the non-hex scheme name).Before
After
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Both fixes are visible on the same time-comparison AG Grid table. In every screenshot the Customize tab has Show summary ✓ enabled, so the only difference is the fix.
28.4k)Before
After
TESTING INSTRUCTIONS
AG_GRID_TABLE_ENABLEDfeature flag.ADDITIONAL INFORMATION
Verified against a local build on apache
masterwithag-grid-community36.0.0. The summary row's computedopacitywas0(from.ag-opacity-zero) before the fix; the Green/Red formatters were computed but had no consumer in the renderer.