kokhlo commented on PR #44665:
URL: https://github.com/apache/superset/pull/44665#issuecomment-5898028327
All four of the substantive points are addressed in `74eccab`, and the
duplicated-fixture note in `faa58ea`. @rusackas @sadpandajoe — you were both
right, and in one case the cause was further downstream than the line you
flagged.
`getValueRange` now coerces whenever bars will be drawn, not only when a
CELL_BAR rule exists, so range and render read the same values. @rusackas your
`generalShowCellBars || hasCellBarFormatter` framing was the right shape; I
passed the coercion unconditionally instead, because the gate above it already
decides whether bars draw at all.
@codeant-ai-for-open-source[bot] and @bito-code-review[bot] were right about
the same line and I had read the symptom rather than the mechanism.
@Copilot the `parseNumeric` point is right: `Number("Infinity")` is not NaN,
so a single non-finite cell stretched the range to `[10, Infinity]` and
flattened every other bar to zero width. Now guarded on both the native-number
and the parsed-string branch.
@Copilot on the description overclaiming — the scope note was a fair catch,
but the underlying behaviour was still wrong rather than merely misdescribed,
so I fixed the behaviour instead of rewording it.
@Copilot the lazy-evaluation point I did not take. `parseNumeric` is a
typeof plus a `Number()` on a trimmed string, the column is already rendering
one styled cell per value, and gating it would put a branch on the hot path to
save work that is not measured to matter. Happy to be argued out of it.
@sadpandajoe the click-to-filter point was correct and is now per-cell. A
CELL_BAR rule reaches only the cells it matches, so the guard moved from the
column-wide `valueRange` to a per-cell `cellDrawsBar`, shared with the render
condition so the two cannot drift.
@sadpandajoe the DECIMAL-as-string metric is the one that turned out to be
two bugs. You correctly identified that the rule fell through to the full-width
band because `isMetric` requires every value to be a number. An explicit
CELL_BAR rule now opens that gate. But the band was only the visible half: the
bar element carried its width in a `css` prop, and this repository's own
Jest/Babel config does not wire up the emotion JSX pragma, so under test the
bar rendered with no geometry whatsoever. Every assertion anyone had written
against this bar — including the one you asked me to strengthen — could only
see its presence. It is an inline style now, matching the arrow a few lines
below, which had already been moved off `css` for the same reason. Your request
to assert a scaled width is what surfaced this; asserting on width was not
possible before.
@codeant-ai-for-open-source[bot] the string-comparator fix needed one more
step than the snippet suggests: `formatter.operator` was never copied into
`ColorFormatters` by `getColorFormatters`, so a guard on it would have been
permanently false and the fix a no-op. The operator now rides along on the
formatter, and `BeginsWith`/`EndsWith`/`Containing`/`NotContaining` receive the
original string.
Each of these is pinned by a test that goes red when the fix is reverted,
verified by mutation rather than by inspection. Six new cases, 106 passing.
One thing I did not do: the bar's numeric coercion still runs once per cell
rather than lazily. That is @Copilot's third point, and I left it for the
reason above.
--
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]
