kokhlo commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4171769632
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts:
##########
@@ -534,15 +578,30 @@ export const getColorFormatters:
MemoizedFn<GetColorFormatters> = memoizeOne(
config?.targetValueRight !== undefined
: config?.targetValue !== undefined)))
) {
+ // A rule that reads its cell as a magnitude has to see the same
+ // numbers the cell renderer compares. Handing it the raw string
+ // leaves the percentage bounds without a column to measure against
+ // — every value is filtered out as non-numeric, so the rule falls
+ // back to automatic bounds and paints a different gradient than the
+ // same data delivered as numbers.
+ const columnValues = data.map(row => {
+ const value = row[config.column!];
+ if (!comparesNumerically(config)) {
+ return value as number;
+ }
+ return parseNumericValue(value) ?? (value as number);
Review Comment:
Right, and this one was mine — the coercion kept the raw text whenever a
parse failed, so a single `"N/A"` put `"N/A"` itself into the bounds domain.
`Math.min` over `[10, 20, "N/A"]` is `NaN`, the automatic range came back
unusable, and `Comparator.None` then dropped every finite row from it — no
color at all, solid or otherwise.
The column values are now the parsed magnitudes only, so a cell with no
number is simply absent from the domain instead of poisoning it:
```
const columnValues = comparesNumerically(config)
? data
.map(row => parseNumericValue(row[config.column!]))
.filter((value): value is number => value !== undefined)
: data.map(row => row[config.column!] as number);
```
`0b2660d` — two regression tests: one asserts `10` and `20` still take the
solid color beside an `"N/A"`, the other that the gradient with the gap matches
the gradient without it.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1299,37 +1375,59 @@ 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,
+ // 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;
+
+ // A styled component rather than the `css` prop or an inline
+ // `style`: the `css` prop needs the emotion JSX pragma, which this
+ // codebase's own Jest/Babel config does not wire up, and an inline
+ // style would outrank every dashboard stylesheet. Both matter here —
+ // the `cell-bar` classes exist precisely so saved custom CSS can
+ // restyle the bar, and a `styled` div keeps that override working
+ // while still emitting a real rule a test can read back.
+ // A background rule paints the cell itself, so the bar the global
+ // toggle would draw over that background is dropped. A CELL_BAR rule
+ // matching the same cell is a separate instruction and keeps its own
+ // geometry: clearing the flag for the background rule must not
flatten
+ // a cell bar into the full-width band, which would make every
matching
+ // value read as the same magnitude.
+ const barHasGeometry =
+ !!valueRange &&
+ numericValue !== undefined &&
+ (valueRangeFlag || !!backgroundColorCellBar);
+ const barGeometry = barHasGeometry
+ ? {
+ width: `${cellWidth({
+ value: numericValue!,
+ valueRange: valueRange!,
alignPositiveNegative,
- })}%`};
- left: ${`${cellOffset({
- value: value as number,
- valueRange,
+ })}%`,
+ left: `${cellOffset({
+ value: numericValue!,
+ valueRange: valueRange!,
alignPositiveNegative,
- })}%`};
- background-color: ${
+ })}%`,
+ backgroundColor:
backgroundColorCellBar ||
cellBackground({
- value: value as number,
+ value: numericValue!,
colorPositiveNegative,
theme,
- })
- };
- `
- }
- `;
+ }),
+ }
+ : backgroundColorCellBar
+ ? {
Review Comment:
Correct, and the gate was wider than the thing it was written for. It
withholds the range from the *generic* bar, which is right — the comparison
colors own the cell background there. An explicit CELL_BAR rule is a separate
instruction on the same cell, and it measures the same magnitudes, so
withholding the range from it too sent every match to the unscaled fallback:
100 and 110 both drawn at 100%.
The gate now exempts an explicit rule on the column it points at:
```
const valueRange =
(!hasBasicColorFormatters || hasCellBarFormatter) &&
(generalShowCellBars || hasCellBarFormatter) &&
```
`0b2660d` — with the global toggle alone the column still draws no bar (that
path is unchanged); with a rule on it, the test asserts the widths are `[91,
100]` against the column's positive extent, where the fallback gave `[100,
100]`.
--
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]
