fix(plugin-chart-echarts): hide value labels on zero-height stacked segments - #42854
fix(plugin-chart-echarts): hide value labels on zero-height stacked segments#42854krishn1301 wants to merge 2 commits into
Conversation
…egments thresholdValues[dataIndex] is 0 when percentage_threshold is 0, which is falsy, so the guard fell back to Number.MIN_SAFE_INTEGER and every value passed. A zero-value series occupies no space in a stacked chart, so its label rendered on top of the adjacent segment's label. Skips the label when the value is exactly 0 while leaving negative values and non-zero thresholds untouched. Fixes apache#42702 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dosubot
Bot
added
change:frontend
Code Review Agent Run #2b2655Actionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if ( | ||
| numericValue !== 0 && | ||
| numericValue >= | ||
| (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER) | ||
| (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER) | ||
| ) { |
There was a problem hiding this comment.
Suggestion: The zero-value guard is reached for non-stacked series when legendState marks the series as unselected, because those series bypass the earlier !stack && isSelectedLegend branch. This changes the existing behavior and suppresses zero labels even though the overlap problem only applies to stacked segments. Restrict the zero suppression to stacked series or preserve the non-stacked label path. [incorrect condition logic]
Severity Level: Minor 🧹
- ⚠️ Isolated unstacked series can lose zero-value labels.
- ⚠️ Legend double-click filtering affects label rendering.
- ⚠️ Stacked-chart overlap fix changes unstacked chart behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 473:477
**Comment:**
*Incorrect Condition Logic: The zero-value guard is reached for non-stacked series when `legendState` marks the series as unselected, because those series bypass the earlier `!stack && isSelectedLegend` branch. This changes the existing behavior and suppresses zero labels even though the overlap problem only applies to stacked segments. Restrict the zero suppression to stacked series or preserve the non-stacked label path.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The flagged issue is correct. The current implementation unconditionally suppresses zero-value labels for all series, which incorrectly affects non-stacked charts where zero values should remain visible. To resolve this, the zero-value check should be conditional on whether the chart is stacked. Proposed FixUpdate the condition in if (
(stack && numericValue !== 0 || !stack) &&
numericValue >=
(thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER)
) {
return formatter(numericValue);
}This change ensures that zero-value suppression only applies to stacked series, preserving existing behavior for non-stacked charts. Please let me know if you would like me to check the remaining comments on this PR. superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts |
|
@krishn1301 thanks for the PR! Since there is a visual component to your change, can you add before and after screenshots of your fix into the PR description? |
There was a problem hiding this comment.
Pull request overview
This PR fixes overlapping value labels in stacked ECharts timeseries bar charts by ensuring per-series labels are not rendered for zero-height stacked segments (value 0) when value labels are enabled.
Changes:
- Skip per-series stacked value labels when the numeric value is exactly
0, preventing label overlap on adjacent stacked segments. - Add targeted unit tests covering zero, non-zero, negative, and thresholded stacked label behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts | Updates stacked value-label formatter guard to suppress labels for zero-valued stacked segments. |
| superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts | Adds regression tests for stacked value label rendering at threshold 0 and non-zero thresholds. |
| showValue: true, | ||
| stack: StackControlsValue.Stack, | ||
| onlyTotal: false, | ||
| formatter: (v: any) => String(v), |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42854 +/- ##
=======================================
Coverage 66.37% 66.37%
=======================================
Files 2857 2857
Lines 161048 161050 +2
Branches 37046 37048 +2
=======================================
+ Hits 106889 106891 +2
Misses 52143 52143
Partials 2016 2016
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
SUMMARY
Fixes #42702.
On a stacked timeseries bar chart, a series whose value is
0occupies no vertical space, but its value label was still rendered — landing on top of the label of the segment next to it.The root cause is a falsy check, not the label logic itself. In
transformSeries:When
percentage_thresholdis0(the default),thresholdValues[dataIndex]is0, which is falsy, so the||falls through toNumber.MIN_SAFE_INTEGERand every value passes the guard, including0.This skips the label when the value is exactly
0, before the threshold comparison.I used
numericValue !== 0rather than thenumericValue > 0suggested in the issue:> 0would also hide labels on negative segments, which do occupy space and legitimately need a label. There's a regression test covering that case.TESTING INSTRUCTIONS
Four tests were added under
transformSeries › stacked value labels:0(fails without this change)0> 0regression)Manually: create a Stacked Timeseries Bar chart with two metrics where one returns
0for some x-values, turn on Show Value, and confirm the0labels no longer overlap the adjacent labels.ADDITIONAL INFORMATION