kokhlo commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4143237618
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1188,37 +1237,55 @@ export default function TableChart<D extends DataRecord
= DataRecord>(
};
`;
- const cellBarStyles = css`
- position: absolute;
- height: 100%;
- display: block;
- top: 0;
- ${
- valueRange &&
- typeof value === 'number' &&
- valueRangeFlag &&
- `
- width: ${`${cellWidth({
- value: value as number,
- valueRange,
- alignPositiveNegative,
- })}%`};
- left: ${`${cellOffset({
- value: value as number,
- valueRange,
- alignPositiveNegative,
- })}%`};
- background-color: ${
- backgroundColorCellBar ||
- cellBackground({
- value: value as number,
- colorPositiveNegative,
- theme,
- })
- };
- `
- }
- `;
+ // Whether this particular cell draws a bar. A CELL_BAR rule can
match
+ // some cells of a column and not others, so the bar — and with it
the
+ // click-to-filter suppression, which exists because the bar overlay
+ // would swallow the click — has to be decided per cell rather than
+ // from the column-wide valueRange.
+ const cellDrawsBar =
+ (generalShowCellBars ? !!valueRange : false) ||
+ !!backgroundColorCellBar;
+
+ // Inline style for the same reason as the arrow below: the `css`
prop
+ // needs the emotion JSX pragma, which this codebase's own Jest/Babel
+ // config does not wire up. As a `css` block the bar's geometry was
+ // silently dropped under test, so bar presence was the only thing
+ // any assertion could see — including for a bar that had no width.
+ const barHasGeometry =
+ !!valueRange && numericValue !== undefined && valueRangeFlag;
+ const cellBarStyles: CSSProperties = {
+ position: 'absolute',
+ height: '100%',
+ display: 'block',
+ top: 0,
+ ...(barHasGeometry
+ ? {
+ width: `${cellWidth({
+ value: numericValue!,
+ valueRange: valueRange!,
+ alignPositiveNegative,
+ })}%`,
+ left: `${cellOffset({
+ value: numericValue!,
+ valueRange: valueRange!,
+ alignPositiveNegative,
+ })}%`,
+ backgroundColor:
+ backgroundColorCellBar ||
+ cellBackground({
+ value: numericValue!,
+ colorPositiveNegative,
+ theme,
+ }),
+ }
+ : backgroundColorCellBar
Review Comment:
Right, and it is the same conflation that produced the two reports above:
one flag was carrying two independent decisions.
A background rule paints the cell, so the bar the global `show_cell_bars`
toggle would draw *over that background* has to go — that is what the flag was
for. But a CELL_BAR rule matching the same cell is a separate instruction, and
clearing the flag for the background rule also stripped the geometry from a bar
that a rule explicitly asked for. The bar then took the `backgroundColorCellBar
? { width: '100%' }` fallback, so every matching value drew the same full-width
band and the column read as flat magnitude — which is what you saw at line 1281.
Fixed in 96c6036959: `barHasGeometry` now requires `valueRangeFlag ||
!!backgroundColorCellBar`. The background rule still drops the toggle-driven
bar, and a cell bar keeps its scale whenever a rule matched it. The full-width
band survives where it belongs — a matched cell bar on a value with no range or
no numeric reading still cannot be scaled, and still gets the band.
New test pairs a background rule matching every cell with a cell-bar rule
over the same column, so the flag is cleared on every cell, and asserts widths
stay 0 / 40 / 100 across `[0, 4000, 10000]`. RED on the previous head: 100 /
100 / 100. Full `plugin-chart-table` suite is 185 passing across 10 suites; the
chart-controls suites are 407 across 44.
One note on scope: the fix is deliberately in the geometry gate rather than
in `applyFormatter`, so rule order does not matter — an ENTIRE_ROW background
rule running after the per-column cell-bar rule lands in exactly the same place.
--
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]
