-
Notifications
You must be signed in to change notification settings - Fork 14.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(plugin-chart-echarts): invalid total label location for negative values in stacked bar chart #21032
fix(plugin-chart-echarts): invalid total label location for negative values in stacked bar chart #21032
Conversation
How does stacked bar chart work when a series group has a mix of negative and positive values? Is the total above the bars only the positive part or a sum of both positives and negatives? Maybe we need to display both negative totals and positive totals separately, with positive totals on top of positive bars (if there are positive sums in a series) and negative totals below negative bars (if there are negatives). |
Displaying two total numbers can be confused. (because user expects to have one total value which means sum of positives and negatives) ++ user can highlight the each negative and positive value by the tooltip |
Codecov Report
@@ Coverage Diff @@
## master #21032 +/- ##
==========================================
- Coverage 66.28% 66.28% -0.01%
==========================================
Files 1770 1770
Lines 67497 67505 +8
Branches 7171 7177 +6
==========================================
+ Hits 44738 44743 +5
Misses 20928 20928
- Partials 1831 1834 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
You can argue both ways, visually it's not very intuitive that a taller bar may have smaller total number on top of it only because it also has a negative part. But I agree the actual totals may indeed be more useful in most cases. |
Here is a related issue I had reported(not for stack bars): I think that in case of negative value, the label should be at the bottom of the bar not at the top of the bar. User would expect the label to be at the end of the bar, which would be the TOP for positive and BOTTOM for negative values. Please consider. |
@kamalkeshavani-aiinside I understand the position for the negative values but the problem is when values are combined with positives and negatives. In this case we should choose either top or bottom because echart only accepts one position for one series. We can design the position to BOTTOM when most of combined values are negatives. Other solution is using a default position(which is
Which option seems better? |
@justinpark Thanks for the clarification. Personally, I prefer 1st option (TOP or BOTTOM based on most values), but 2nd option can be useful for others. |
@ktmud I agree that the best way to stack when positive and negative values are present is to have the negative values stacked below the line and respectively the positive ones above the line. I have a slightly related fix that I need to get in that's related to bumping ECharts to 5.3.3, so I'll see if I can tackle that at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor nit regarding the storybook (thanks for adding it btw!)
chartType="echarts-timeseries" | ||
width={width} | ||
height={height} | ||
queriesData={[{ data: negativeNumData }]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the storybook isn't formatting the temporal x-axis correctly:
Could we add an entry for __timestamp
to colnames
and coltypes
which would normally be returned in the chart data payload so the formatting looks correct (2 comes from GenericDataType.TEMPORAL
)? I believe this should fix it:
queriesData={[{ data: negativeNumData }]} | |
queriesData={[{ data: negativeNumData, colnames: ['__timestamp'], coltypes: [2] }]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for hotfix! I'll update accordingly
f60d21e
to
60a2e8e
Compare
Since villebro has a plan to fix the negative number position issue, I'll move forward as-is. |
…values in stacked bar chart (apache#21032)
…values in stacked bar chart (apache#21032) (cherry picked from commit bb4ab9b)
…values in stacked bar chart (apache#21032) (cherry picked from commit a8ba544)
SUMMARY
A stacked chart sets the total value only, the total label displays on top of the most latest non-null data index item.
When the data contains negative values, the stack chart places the item underneath the positive item.
Therefore the total value can be located within the stack bar when a negative value in the most latest index.
This commit fixes the
extractShowValueIndexes
logic by ignoring the negative values for onlyTotal option.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
Storybook
ADDITIONAL INFORMATION