fix(chart): STACKED BARS VALUES did not show correctly when data has … #19006
Conversation
|
@villebro hi, are you available to review the code? |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #19006 +/- ##
==========================================
- Coverage 66.58% 65.18% -1.40%
==========================================
Files 1641 2103 +462
Lines 63548 90599 +27051
Branches 6424 13426 +7002
==========================================
+ Hits 42312 59061 +16749
- Misses 19555 28404 +8849
- Partials 1681 3134 +1453
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@zhoulijuanmiao Thanks for the fix! I will test it in my local environment. |
|
Hi @zhoulijuanmiao, Could you double-check this PR? I haven't got any change on the PR.
|
|
hi @zhaoyongjie, before i fixed it, the chart was looked like this，could you ignore my change in local environment and check again? |
|
Hi @zhoulijuanmiao, Thanks for your fix, and I have checked in your PR. Currently, the viz looks good. But it looks like there is an issue(might be too short) with all the negative values in the Viz. After your PR
BTW, I found a workaround for showing labels is that set Before your PR
|
|
hi @zhaoyongjie, Thanks for your check, seems like I need to explain the pr a bit more specific, as you can see, before my pr, there is a feature of how to show bar value labels, negative value showing on the bottom of the bar, positive value showing on the top of the bar. The rule is satisfied in unstacked bar chart, to see it clearly, I set filters in all pictures below
But the rule is not satisfied in stacked bar chart, my purpose is to fix it
After my pr, the rule will be satisfied in stacked bar chart, additionally, this may make the viz looks better
|
|
@zhaoyongjie hi, any progress? |
|
Tagging @villebro for review since he's played around with stacking negative values in this chart before. |
|
Sorry this never got followed up on @ppjmiao — I'll add @michael-s-molina to the conversation, who recently looked at pos/neg value support in this chart quite closely. I'll also close/reopen this PR to retrigger CI and see if we can get it to pass. |
There was a problem hiding this comment.
At the line 80, there is repetitive code svg.selectAll('.bar-chart-label-group').remove();, I think we can prune this code on this PR together.
svg.selectAll('.bar-chart-label-group').remove();
setTimeout(() => {
svg.selectAll('.bar-chart-label-group').remove();
.....
}
| const yPos = parseFloat(rectObj.attr('y')); | ||
| const rectWidth = parseFloat(rectObj.attr('width')); | ||
| const rectHeight = parseFloat(rectObj.attr('height')); | ||
| const isPositive = rectObj.attr('class').includes('positive'); |
There was a problem hiding this comment.
I think we no need isPositive variable
|
Seems the last couple comments on this PR have gone unaddressed for the last 10 months or so. Please advise :D |
@rusackas looks like the initial problem that this pr addresses has been fixed, i'd vote to close out this PR |
…negative value
fixed the bug #18996
before:
after:
