sadpandajoe commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4171590570
##########
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:
For a cell-bar rule with no comparator over a string source column
containing `"10"`, `"20"`, and `"N/A"`, this produces `[10, 20, "N/A"]`; the
automatic min/max become `NaN`, so the finite rows no longer match and lose
their bars even with a solid color. Could failed numeric parses be excluded
from the numeric bounds domain rather than retained here?
##########
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:
With time comparison and comparison colors enabled,
`hasBasicColorFormatters` prevents `valueRange` from being calculated, so an
explicit cell-bar rule renders values such as 100 and 110 as identical
100%-width bands. Could explicit cell-bar rules retain their numeric range in
this mode instead of taking the unscaled fallback?
--
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]
