sadpandajoe commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4134324567
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1050,11 +1071,23 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
basicColorFormatters.length > 0;
const generalShowCellBars =
config.showCellBars === undefined ? showCellBars : config.showCellBars;
+ // A Cell bar conditional-formatting rule must keep working even when the
+ // generic "Show cell bars" toggle is off: the toggle controls the
default
+ // gradient, not whether an explicit formatter rule can draw its bar.
+ const hasCellBarFormatter =
+ hasColumnColorFormatters &&
+ columnColorFormatters.some(
+ formatter =>
+ formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR &&
+ (formatter.columnFormatting
+ ? formatter.columnFormatting === key
+ : formatter.column === key),
+ );
const valueRange =
!hasBasicColorFormatters &&
- generalShowCellBars &&
+ (generalShowCellBars || hasCellBarFormatter) &&
Review Comment:
Since `valueRange` now also turns on whenever any single value in the column
matches a CELL_BAR rule (`hasCellBarFormatter`), it turns on for the whole
column, not just the matching cell. The existing cross-filter `onClick` handler
is gated on `!valueRange`, so with `emitCrossFilters` enabled and the global
toggle off, adding a CELL_BAR rule to a raw-record column silently disables
click-to-filter on every cell in that column, including cells the rule doesn't
match and that never render a bar. Should the click guard use a per-cell
condition instead of this column-level `valueRange`?
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1195,29 +1244,38 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
top: 0;
${
valueRange &&
- typeof value === 'number' &&
+ numericValue !== undefined &&
valueRangeFlag &&
`
width: ${`${cellWidth({
- value: value as number,
+ value: numericValue,
valueRange,
alignPositiveNegative,
})}%`};
left: ${`${cellOffset({
- value: value as number,
+ value: numericValue,
valueRange,
alignPositiveNegative,
})}%`};
background-color: ${
backgroundColorCellBar ||
cellBackground({
- value: value as number,
+ value: numericValue,
colorPositiveNegative,
theme,
})
};
`
}
+ ${
+ !(valueRange && numericValue !== undefined && valueRangeFlag) &&
Review Comment:
This fallback fires whenever a CELL_BAR rule assigns a color but
`valueRange`/`numericValue` didn't resolve — which also happens for a
legitimate numeric metric returned as a string in aggregate mode: `isMetric`
requires every row's value to be `typeof 'number'`, so a DECIMAL-as-string
metric column stays `isMetric: false`, and with
`isRawRecords`/`isPercentMetric` also false the range gate above never opens. A
matching rule then paints a full-width band instead of a proportionally scaled
bar for exactly the numeric-as-string case this PR is meant to fix. Should
aggregate metric columns get the same numeric coercion this PR added for raw
records?
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -1031,6 +1031,97 @@ describe('plugin-chart-table', () => {
cells = document.querySelectorAll('td');
});
+ test('renders a bar for a cell-bar conditional formatting rule
regardless of the global toggle', () => {
+ const baseProps = (showCellBars: boolean) =>
+ transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['num'],
+ coltypes: [GenericDataType.Numeric],
+ data: [{ num: 1234 }, { num: 10000 }, { num: 0 }],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: showCellBars,
+ conditional_formatting: [
+ {
+ colorScheme: '#ACE1C4',
+ column: 'num',
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ },
+ ],
+ },
+ });
+
+ const getBars = (props: ReturnType<typeof baseProps>) => {
+ const { container } = render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+ const rows = container.querySelectorAll('tbody tr');
+ const bars: (Element | null)[] = [];
+ rows.forEach(row => {
+ bars.push(row.querySelector('td div.cell-bar'));
+ });
+ return bars;
+ };
+
+ // Toggle ON: bars render everywhere, including the matched cell.
+ const barsOn = getBars(baseProps(true));
+ expect(barsOn[0]).toBeTruthy();
+ expect(barsOn[1]).toBeTruthy();
+
+ // Toggle OFF: only the cell matching the rule draws a bar;
+ // non-matching cells stay bare.
+ const barsOff = getBars(baseProps(false));
+ expect(barsOff[0]).toBeTruthy();
+ expect(barsOff[1]).toBeNull();
+ expect(barsOff[2]).toBeNull();
+ });
+
+ test('cell-bar rule on string numeric cells matches the comparator
numerically', () => {
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['num'],
+ coltypes: [GenericDataType.Numeric],
+ data: [{ num: '1234.00' }, { num: '10000.00' }, { num: '0.00' }],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: false,
+ conditional_formatting: [
+ {
+ colorScheme: '#ACE1C4',
+ column: 'num',
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ },
+ ],
+ },
+ });
+ const { container } = render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+ const rows = container.querySelectorAll('tbody tr');
+ const bar0 = rows[0].querySelector('td div.cell-bar');
+ const bar1 = rows[1].querySelector('td div.cell-bar');
+ expect(bar0).toBeTruthy();
+ expect(bar1).toBeNull();
Review Comment:
This test only asserts bar presence/absence, not width, so it would still
pass if the range/geometry never resolved and the bar instead rendered through
the new full-width fallback (100% width) rather than a proportionally scaled
one. Can this assert a scaled width on the matching cell instead of just
truthiness?
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1140,6 +1176,19 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
} else {
valueToFormat = value;
}
+ // String cells that read as numbers ("1.00") must compare
+ // numerically, or comparator rules like `= 1` never match.
+ if (
+ formatter.objectFormatting ===
+ ObjectFormattingEnum.CELL_BAR &&
+ valueToFormat !== null &&
+ valueToFormat !== undefined
+ ) {
+ const coerced = parseNumeric(valueToFormat);
+ if (coerced !== undefined) {
+ valueToFormat = coerced;
+ }
+ }
Review Comment:
Agreed—coercing every CELL_BAR formatter's value to a number before calling
into the comparator means `isString(value)` is false whenever the value parses
as numeric, so `BeginsWith`/`EndsWith`/`Containing`/`NotContaining` rules on
numeric-looking string columns stop matching after this change. Should the
coercion be skipped when the rule's operator is one of the string comparators?
--
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]
