kokhlo commented on code in PR #44665:
URL: https://github.com/apache/superset/pull/44665#discussion_r4140676303
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1050,11 +1084,29 @@ 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),
+ );
+ // Render geometry parses numeric-looking strings, so the range has to be
+ // built from the same values or a string cell gets a width measured
+ // against a range that excluded it. An explicit CELL_BAR rule also lifts
+ // the numeric-column gate: a DECIMAL metric delivered as a string is not
+ // flagged isMetric, and it still needs proportionally scaled bars rather
+ // than a full-width band.
const valueRange =
!hasBasicColorFormatters &&
- generalShowCellBars &&
- (isMetric || isRawRecords || isPercentMetric) &&
- getValueRange(key, alignPositiveNegative);
+ (generalShowCellBars || hasCellBarFormatter) &&
+ (isMetric || isRawRecords || isPercentMetric || hasCellBarFormatter) &&
+ getValueRange(key, alignPositiveNegative, true);
Review Comment:
Both right, and this one was mine — the coercion was unconditional across
every raw-record column, so `"00123"` became a magnitude and grew a bar. The
bar overlay is also what suppresses the cross-filter click, so the column lost
filtering as well; that part of your report is the more expensive half.
Fixed in `ddda026`. Coercion is now gated on the column being declared
numeric (`dataType === GenericDataType.Numeric`) **or** carrying an explicit
cell-bar rule. The second half of the gate is deliberate: a DECIMAL metric
delivered as strings is not flagged `isMetric` and the backend can report its
type as text, but it is still a magnitude and wants proportionally scaled bars
rather than the full-width fallback band. A bare string column under the
generic toggle is left alone.
New test renders a `GenericDataType.String` column of `'00123' / '00456' /
'00789'` with the toggle on: zero bars, and a click on the first cell still
emits the cross-filter for `code: ['00123']`. RED first — on the previous head
it drew 3 bars.
I took the second half of your sentence (`DECIMAL values returned as
strings`) as the requirement rather than dropping to native numbers only; if
you'd rather have the gate be `dataType` alone and accept the full-width band
for a mis-reported DECIMAL, say so and I'll narrow it.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -1140,6 +1195,29 @@ 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.
+ // Two kinds of rule are the exception, because both match on
+ // the string itself and parsing first would hide the value
+ // from them: a text comparator, and any rule whose target the
+ // control stored as text (a text column renders a text input,
+ // and Equal compares that target strictly).
+ if (
+ formatter.objectFormatting ===
+ ObjectFormattingEnum.CELL_BAR &&
+ !(
+ formatter.operator !== undefined &&
+ STRING_COMPARATORS.has(formatter.operator)
+ ) &&
+ typeof formatter.targetValue !== 'string' &&
+ valueToFormat !== null &&
+ valueToFormat !== undefined
+ ) {
+ const coerced = parseNumeric(valueToFormat);
Review Comment:
Correct, and the `as number` cast was hiding it — it asserts a type the
values don't have at runtime, so `resolvePercentBound`'s `typeof value ===
'number'` filter saw an empty column and handed back `undefined`, i.e. the
automatic range.
Fixed in `ddda026` by making the formatter receive the numbers its own rule
compares: the parse and the "is this rule numeric" predicate now live next to
the rule engine in `getColorFormatters.ts`, and both the table renderer and the
bound resolution read them, so the bar geometry and the bound can no longer
disagree about what the cell is.
New test builds the same `BoundUnit.Percent` cell-bar rule twice over
identical magnitudes — once as `[50, 100]`, once as `['50.00', '100.00']` — and
asserts both formatters hand back the same colors. RED on the previous head:
`#FF000000` for the string column against `#FF000040` for the numeric one,
which is exactly the different gradient you described.
One thing worth flagging for your review: the predicate deliberately keeps
text comparators and string-persisted targets uncoerced, so their bounds still
fall back. That is intended — those rules match on the string itself, and a
magnitude bound over them is meaningless — but it does mean "percentage bounds
on a cell-bar rule" is only resolved for the numeric subset.
--
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]
