amaannawab923 commented on code in PR #42115:
URL: https://github.com/apache/superset/pull/42115#discussion_r3601296316
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -717,9 +720,32 @@ const transformProps = (
const passedData = isUsingTimeComparison ? comparisonData || [] : data;
const passedColumns = isUsingTimeComparison ? comparisonColumns : columns;
- const basicColorFormatters =
+ // Increase/decrease formatters from the "Comparison color" toggle, keyed by
+ // metric column key.
+ const comparisonColorFormatters =
comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
+ // Custom conditional-formatting rules using the Green (increase) / Red
+ // (decrease) color scheme on a time-comparison table. These were computed
but
+ // never consumed by the AG Grid renderer, so the colors/arrows never showed
+ // (the classic plugin-chart-table does consume them). Route them through the
+ // same increase/decrease path, keyed by metric column key (see above).
+ const basicColorColumnFormatters = getBasicColorFormatterForColumn(
+ baseQuery?.data,
+ columns,
+ conditionalFormatting,
+ );
+
+ // Merge both per-row into a single map so the existing row-attached
formatter
+ // drives the renderer for either source.
+ const basicColorFormatters =
+ comparisonColorFormatters || basicColorColumnFormatters
+ ? (baseQuery?.data ?? []).map((_row, index) => ({
+ ...(comparisonColorFormatters || [])[index],
+ ...(basicColorColumnFormatters || [])[index],
+ }))
+ : comparisonColorFormatters;
Review Comment:
Thanks, the underlying concern is valid but the fix belongs in
`getCellStyle`, not the merge.
The merge itself does not add `undefined` keys: spreading `undefined`/`null`
in an object literal is a no-op, so `{ ...a[i], ...b[i] }` only carries keys
that actually exist on each source. The real clobber was in `getCellStyle`,
which unconditionally overwrote `backgroundColor` with the basic
(increase/decrease) formatter color. That value is `undefined` for a column
without a Green/Red rule, so a column carrying only a standard
conditional-format rule lost its background whenever Green/Red rules existed on
other columns.
Fixed in afe9c90 by only applying the basic background when the column
actually has an increase/decrease formatter. Standard conditional formatting is
preserved, and comparison-color mode is unaffected (it builds a formatter for
every metric column).
I did not take the suggested merge change: always mapping to `{}` per row
makes `basicColorFormatters` non-empty on every time-comparison table, which
flips `hasBasicColorFormatters` on universally and would clobber all standard
CF and comparison color, not just the Green/Red case. Keeping the `... ? map :
false` guard is what stops `hasBasicColorFormatters` from turning on when there
are no formatters at all.
--
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]
