MannXo opened a new pull request, #42756:
URL: https://github.com/apache/superset/pull/42756
### SUMMARY
Fixes #42702.
On a stacked `echarts_timeseries_bar` with "Show Value" on and "Only Total"
off, a series whose value is `0` for a category still gets a value label. A
zero-height stacked segment begins and ends at the same coordinate as the top
of the segment beneath it, so echarts draws that label on top of the label
belonging to the segment below, and the two numbers render over each other as
unreadable text.
This adds one guard: a stacked segment with no height carries no label.
**One correction to the root cause in the issue.** The issue reports that
with the default `percentage_threshold: 0` the condition becomes `numericValue
>= 0`. It does not. `0` is falsy, so `thresholdValues[dataIndex] ||
Number.MIN_SAFE_INTEGER` evaluates to `Number.MIN_SAFE_INTEGER` and the
condition is `numericValue >= Number.MIN_SAFE_INTEGER`, which is true for
everything. Observed against the label formatter:
```
thresholdValues=[0] 0 -> "0" -5 -> "-5" 32 -> "32"
thresholdValues=[10] 0 -> "" 5 -> "" 50 -> "50"
```
If the mechanism were `>= 0`, that `-5` would have been dropped. So the `||`
is doing its job: a `0` threshold means "no threshold filtering", and a real
threshold already suppresses zeros. That makes this a label-collision bug
rather than a threshold bug, and it is why the fix does not touch the threshold
expression. Rewriting that comparison would change threshold semantics for
every user and would suppress legitimate negative labels.
**A second defect on the same code path.** A `null` value renders the
literal string `"null"` as a chart label, with the production number formatter:
```
0 -> "0" null -> "null" 32 -> "32" -5 -> "-5"
```
A null segment has no height either, so it collides identically. The guard
covers both under one rule rather than special-casing zero.
**Prior art in this plugin.** `Timeseries/transformProps.ts` already omits
zero observations from the rich tooltip of a stacked series (`if
(value.observation === 0 && stack) return;`). This applies the same judgement
to the per-series value label.
Scope: stacked series only. Unstacked labels sit on the bar itself with
nothing to collide with, and they return earlier in the formatter. Only-total
labels render the stack total and are unaffected. The `stack &&` clause is
deliberate for legibility even though the earlier `!stack` return makes it
defensive rather than load-bearing, matching how the tooltip guard above spells
out the same condition.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Stacked bar, two metrics, "Show Value" on, "Only Total" off, default
percentage threshold, with the upper series at `0` for two categories.
<!-- BEFORE -->
<!-- AFTER -->
The label coordinates from the rendered SVG, so the images and the numbers
agree:
```
before after
x=146.3 y=143.0 '0' x=146.3 y=143.0 '32'
x=146.3 y=143.0 '32' x=775.2 y=179.2 '27'
x=775.2 y=179.2 '0'
x=775.2 y=179.2 '27'
overlapping pairs: ('0','32'), ('0','27') overlapping pairs: none
```
### TESTING INSTRUCTIONS
Manual:
1. Create an `echarts_timeseries_bar` chart with two metrics, for example
`A` and `B`.
2. Set Stacked, turn on "Show Value", leave "Only Total" off and "Percentage
threshold" at its default.
3. Include a category where the metric stacked on top is `0` and the one
below it is not, for example `{"category": "1-3g", "B": 32, "A": 0}`.
4. Run the chart. Before this change the `0` label is drawn over the `32`
label as doubled text. After it, only `32` is labelled.
5. Confirm the unchanged cases: a category where both metrics are non-zero
still labels both segments; switching off Stacked still labels a `0`; turning
on "Only Total" still shows the stack total.
Automated, in
`plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts`:
```
npx jest plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
```
Five cases cover zero and null suppressed when stacked, non-zero and
negative kept, zero kept when not stacked, `percentage_threshold` still
filtering below-threshold values, and only-total labels unchanged.
Gates run locally on Node 24.16.0, using the commands from
`.github/workflows/superset-frontend.yml`:
```
npx jest plugins/plugin-chart-echarts 69 suites, 772 tests, all passing
npm run lint clean
npm run plugins:build && npm run type clean
npm ls --all --package-lock-only --depth=0 clean
pre-commit run --files <the two changed files> all hooks passing
```
Each new assertion was checked by reverting the behaviour it covers and
confirming the intended test fails: removing the guard fails the zero-height
case, and guarding zero without null fails it too. One further sabotage,
removing the `stack &&` clause, is *not* caught, because the `!stack` branch
returns before the guard is reached, so no test can distinguish it. Noting that
rather than presenting the set as complete.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #42702
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
