-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
feat: pie-chart support negative value #15095
Conversation
Thanks for your contribution! |
@ssthouse Could you try to filter negative data with pie series in |
I'll try~ |
@Ovilia hi, do you mean adding filter logic like this? So I can discard the changed I've made in |
@ssthouse Should register a new processor to filter the negative data. See https://github.com/apache/echarts/blob/master/src/chart/pie/install.ts#L36 about how to register a processor |
@pissang hi, I create a new filter: |
It seems this will break the case in which all the number is negative. Besides, if the negative value was considered as |
@plainheart hi, In my previous edition(see the first two commits of this PR). according to @Ovilia and @pissang's suggestion, I moved the filter logic to I'm not sure which result is expected since they're both reasonable in some way,hope you guys can give me a conclusion😂 |
I think it's a little better if not show in the tooltip but show in legend. |
test/pie-minus-value.html
Outdated
center: ["50%", "60%"], | ||
selectedMode: "single", | ||
data: [ | ||
{ value: 335, name: "直接访问" }, |
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.
As @plainheart suggested, I think we should add a test case that all data are negative.
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.
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.
When all the data are negative, the pie chart is empty.
I'm not sure if this is the expected result 🐒
At least, it's unexpected for me. This will break the previous behavior (See below). And I think it's meaningful when all value is negative. In that case, considering them as 0
or filtering them may be not a good way.
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.
If the canvas is blank, it looks like there is an error to the users. So perhaps we could display an empty light gray circle when no data (sum is zero or all is '-'
or negative). We should also provide options to configure the color.
The legend color doesn't look right to me. They should have different colors for different series.
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.
@Ovilia this sounds reasonable, I'll give it a try~
display an empty light gray circle when no data
For the color issue, all three legends' are in blue because they are all filtered by negativeDataFilter
. And the filtered legend's default color is blue
3b15503
to
21e2f65
Compare
@pissang Yes, sure~ I have a small question that may need your help. 🙏 About the option I checked the sector's API and found it accepts |
@ssthouse Here is a common way to set style from the option: sector.useStyle(seriesModel.getModel('emptyCircleStyle').getItemStyle()); Some notes:
|
@pissang Thanks for your suggestions, really helpful! I have added the config options, please check~ |
src/processor/negativeDataFilter.ts
Outdated
const legendModels = ecModel.findComponents({ | ||
mainType: 'legend' | ||
}); | ||
if (!legendModels || !legendModels.length) { |
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 think it's not necessary to check if there is legend
?
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.
fixed
src/chart/pie/PieView.ts
Outdated
@@ -242,6 +243,29 @@ class PieView extends ChartView { | |||
} | |||
} | |||
|
|||
// remove empty-circle(which is not instance of PiePiece) if it exists | |||
group.children().forEach(child => { | |||
if (!(child instanceof PiePiece)) { |
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.
It's not a good way to check if it's the empty-circle element by checking it's not a piece of the pie. It may have issues if we add other elements that are not a piece of the pie further.
A better way is to save this empty-circle element as a private property of PieView. It's simpler and more robust.
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.
fixed
ed3ad28
to
876c7b3
Compare
LGTM now. @Ovilia @plainheart Any more comments? |
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 the work. It should be okay generally. But I can't keep confident about the final process: filter the negative values.
Comparing to current behavior, this result looks more expected for me,
That is, leave the negative value there and its percentage is 0.
And if all values are negative, display them as the same as the positive ones. (BTW, this is also the behavior before this PR's changes.)
@plainheart About the negative value filtering. I can accept it mainly because highcharts is using the same strategy. But I'm also not sure if there are developers using this "bug" as a "feature" to get the result they want. If so this change may break it and we need an extra option to choose not to filter these negative values. |
@pissang Should I add this config now or we wait for developers‘ feedback after release? |
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
Congratulations! Your PR has been merged. Thanks for your contribution! 👍 |
在Pie中强制过滤负数,是否简单粗暴了一些,在饼图和圆环图中负数确实导致图形绘制的问题,但玫瑰图可以真实反映负数,是否可以将PIE中"是否过滤负值"作为一个用户选项. |
@yangkai-bj 谢谢建议,欢迎 PR 增加该配置 |
Brief Information
This pull request is in the type of:
What does this PR do?
Support minus value in Pie Chart.
Fixed issues
Details
Before: What was the problem?
the minus value slice fillup the entire pie chart;
After: How is it fixed in this PR?
Misc
Related test cases or examples to use the new APIs
NA.
add test file:
pie-minus-value.html
in test folder;Others
Merging options
Other information