sadpandajoe commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4409347714
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -1469,6 +1553,306 @@ 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 props = (showCellBars: boolean) =>
+ cellBarProps({
+ values: [1234, 10000, 0],
+ showCellBars,
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ }),
+ });
+
+ // Toggle ON: bars render everywhere, including the matched cell.
+ const barsOn = renderCellBars(props(true)).bars;
+ 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 = renderCellBars(props(false)).bars;
+ 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 { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234.00', '10000.00', '0.00'],
+ showCellBars: false,
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ }),
+ }),
+ );
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ // Presence alone would also pass for the full-width fallback, so pin
+ // the geometry: 1234 against a [0, 10000] range is not the whole cell.
+ expect(barWidth(bars[0]!)).toBeLessThan(100);
+ expect(barWidth(bars[0]!)).toBeGreaterThan(0);
+ });
+
+ test('cell-bar rule with a string comparator still matches
numeric-looking cells', () => {
+ const { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234.00', '5678.00'],
+ showCellBars: false,
+ rule: cellBarRule({
+ operator: Comparator.Containing,
+ targetValue: '23',
+ }),
+ }),
+ );
+ // "1234.00" contains "23"; parsing it to 1234 first would hide the
text
+ // from the comparator and drop the bar entirely.
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ });
+
+ test('cell-bar rule with a string target value still matches the cell it
was written for', () => {
+ const { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234', '5678'],
+ showCellBars: false,
+ // A String column renders a text input, so the target the control
+ // persists is a string, and Equal compares it strictly. Parsing
the
+ // cell first would leave the rule unable to match its own cell.
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: '1234',
+ }),
+ }),
+ );
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ });
+
+ test('bar geometry stays inside the cell when a column mixes numbers and
numeric strings', () => {
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['num'],
+ coltypes: [GenericDataType.Numeric],
+ data: [{ num: 50 }, { num: '-50' }, { num: '100' }],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: true,
+ },
+ });
+ const { container } = render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+ const bars = container.querySelectorAll('div.cell-bar');
+ expect(bars.length).toBe(3);
+ bars.forEach(bar => {
+ // The range is built from the same parsed values the renderer uses,
+ // so no cell — string or native — can measure past the column max.
+ const width = barWidth(bar);
+ expect(width).toBeGreaterThanOrEqual(0);
+ expect(width).toBeLessThanOrEqual(100);
+ });
+ });
+
+ test('a cell-bar rule leaves click-to-filter on the cells it does not
match', () => {
+ const setDataMask = jest.fn();
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['name', 'num'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ data: [
+ { name: 'a', num: 1 },
+ { name: 'b', num: 9000 },
+ ],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: false,
+ conditional_formatting: [
+ {
+ colorScheme: '#ACE1C4',
+ column: 'num',
+ operator: Comparator.GreaterThan,
+ targetValue: 5000,
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ },
+ ],
+ },
+ emitCrossFilters: true,
+ });
+ const { container } = render(
+ <ProviderWrapper>
+ <TableChart
+ {...props}
+ emitCrossFilters
+ setDataMask={setDataMask}
+ sticky={false}
+ />
+ </ProviderWrapper>,
+ );
+ const rows = container.querySelectorAll('tbody tr');
+ // Only the second row matches the rule and therefore only it draws a
bar.
+ expect(rows[0].querySelector('div.cell-bar')).toBeNull();
+ expect(rows[1].querySelector('div.cell-bar')).toBeTruthy();
+ // Click the bar column itself: a column-wide gate would have made the
+ // first row's cell unclickable even though it draws nothing.
+ const bareCell = rows[0].querySelectorAll('td')[1];
+ expect(bareCell.querySelector('div.cell-bar')).toBeNull();
+ fireEvent.click(bareCell);
Review Comment:
This test clicks only the unmatched cell, so the other half of the per-cell
gate is untested: if `!cellDrawsBar` were dropped from the `onClick` condition,
the `9000` cell would still emit a cross-filter even though it draws a bar
overlay, and this test would still pass. Could it also click the matching cell
in `rows[1]` and assert that `setDataMask` is not called for it?
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -1469,6 +1553,306 @@ 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 props = (showCellBars: boolean) =>
+ cellBarProps({
+ values: [1234, 10000, 0],
+ showCellBars,
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ }),
+ });
+
+ // Toggle ON: bars render everywhere, including the matched cell.
+ const barsOn = renderCellBars(props(true)).bars;
+ 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 = renderCellBars(props(false)).bars;
+ 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 { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234.00', '10000.00', '0.00'],
+ showCellBars: false,
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: 1234,
+ }),
+ }),
+ );
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ // Presence alone would also pass for the full-width fallback, so pin
+ // the geometry: 1234 against a [0, 10000] range is not the whole cell.
+ expect(barWidth(bars[0]!)).toBeLessThan(100);
+ expect(barWidth(bars[0]!)).toBeGreaterThan(0);
+ });
+
+ test('cell-bar rule with a string comparator still matches
numeric-looking cells', () => {
+ const { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234.00', '5678.00'],
+ showCellBars: false,
+ rule: cellBarRule({
+ operator: Comparator.Containing,
+ targetValue: '23',
+ }),
+ }),
+ );
+ // "1234.00" contains "23"; parsing it to 1234 first would hide the
text
+ // from the comparator and drop the bar entirely.
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ });
+
+ test('cell-bar rule with a string target value still matches the cell it
was written for', () => {
+ const { bars } = renderCellBars(
+ cellBarProps({
+ values: ['1234', '5678'],
+ showCellBars: false,
+ // A String column renders a text input, so the target the control
+ // persists is a string, and Equal compares it strictly. Parsing
the
+ // cell first would leave the rule unable to match its own cell.
+ rule: cellBarRule({
+ operator: Comparator.Equal,
+ targetValue: '1234',
+ }),
+ }),
+ );
+ expect(bars[0]).toBeTruthy();
+ expect(bars[1]).toBeNull();
+ });
+
+ test('bar geometry stays inside the cell when a column mixes numbers and
numeric strings', () => {
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['num'],
+ coltypes: [GenericDataType.Numeric],
+ data: [{ num: 50 }, { num: '-50' }, { num: '100' }],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: true,
+ },
+ });
+ const { container } = render(
+ ProviderWrapper({
+ children: <TableChart {...props} sticky={false} />,
+ }),
+ );
+ const bars = container.querySelectorAll('div.cell-bar');
+ expect(bars.length).toBe(3);
+ bars.forEach(bar => {
+ // The range is built from the same parsed values the renderer uses,
+ // so no cell — string or native — can measure past the column max.
+ const width = barWidth(bar);
+ expect(width).toBeGreaterThanOrEqual(0);
+ expect(width).toBeLessThanOrEqual(100);
+ });
+ });
+
+ test('a cell-bar rule leaves click-to-filter on the cells it does not
match', () => {
+ const setDataMask = jest.fn();
+ const props = transformProps({
+ ...testData.raw,
+ queriesData: [
+ {
+ ...testData.raw.queriesData[0],
+ colnames: ['name', 'num'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ data: [
+ { name: 'a', num: 1 },
+ { name: 'b', num: 9000 },
+ ],
+ },
+ ],
+ rawFormData: {
+ ...testData.raw.rawFormData,
+ show_cell_bars: false,
+ conditional_formatting: [
+ {
+ colorScheme: '#ACE1C4',
+ column: 'num',
+ operator: Comparator.GreaterThan,
+ targetValue: 5000,
+ objectFormatting: ObjectFormattingEnum.CELL_BAR,
+ },
+ ],
+ },
+ emitCrossFilters: true,
+ });
+ const { container } = render(
+ <ProviderWrapper>
+ <TableChart
+ {...props}
+ emitCrossFilters
+ setDataMask={setDataMask}
+ sticky={false}
+ />
+ </ProviderWrapper>,
+ );
+ const rows = container.querySelectorAll('tbody tr');
+ // Only the second row matches the rule and therefore only it draws a
bar.
+ expect(rows[0].querySelector('div.cell-bar')).toBeNull();
+ expect(rows[1].querySelector('div.cell-bar')).toBeTruthy();
+ // Click the bar column itself: a column-wide gate would have made the
+ // first row's cell unclickable even though it draws nothing.
+ const bareCell = rows[0].querySelectorAll('td')[1];
+ expect(bareCell.querySelector('div.cell-bar')).toBeNull();
+ fireEvent.click(bareCell);
+ const crossFilterCall = setDataMask.mock.calls.find(
+ (call: any[]) => call[0]?.filterState?.filters,
+ );
+ expect(crossFilterCall).toBeDefined();
+ expect(crossFilterCall![0].filterState.filters).toEqual({ num: [1] });
+ });
+
+ test('a numeric-as-string metric column gets scaled bars, not a
full-width band', () => {
+ const props = transformProps({
+ ...testData.basic,
+ queriesData: [
+ {
+ ...testData.basic.queriesData[0],
+ colnames: ['name', 'num'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
Review Comment:
The numeric-string fixture declares `GenericDataType.Numeric`, so `dataType
=== GenericDataType.Numeric` already enables coercion and the `||
hasCellBarFormatter` half of `coerceNumericColumn` is never exercised. If that
half were removed, a DECIMAL metric that the backend reports as a string type
would fall back to the full-width band, and this test would still pass. Could
this fixture use `GenericDataType.String` for `num` and keep the 67% assertion?
##########
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) &&
+ backgroundColorCellBar &&
+ `
+ width: 100%;
+ left: 0;
+ background-color: ${backgroundColorCellBar};
+ `
Review Comment:
Agreed—at this head a CELL_BAR rule that matches a cell with no numeric
reading still reaches the `backgroundColorCellBar ? { width: '100%', ... }`
fallback in `TableChart.tsx`. For example, with rows `{status: 'urgent',
amount: null}` and `{status: 'normal', amount: 20}`, a rule on `status =
'urgent'` formatting `amount`, and the global toggle off, the null cell gets a
full-width bar behind its empty value. Should the bar be skipped when the cell
has no numeric value instead of drawing the unscaled band?
--
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]
