-
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): 'itemRadiusScale' function added for pie series #19435
base: master
Are you sure you want to change the base?
Conversation
… the radius of each item
Thanks for your contribution! Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the |
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19435@a0db5b4 |
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.
This is a really fascinating feature and this PR is well-written! Thanks for your contribution. I like this feature but just want to make sure, can you give me a real world usage for this feature. #19020 is a good case but it only uses this feature when one bar is highlighted so it may not be convincing.
src/chart/pie/PieSeries.ts
Outdated
@@ -107,6 +109,7 @@ export interface PieSeriesOption extends | |||
type?: 'pie' | |||
|
|||
roseType?: 'radius' | 'area' | |||
itemRadiusScale?: (dataParams: PieCallbackDataParams) => number |
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 would suggest considering another name than radius
because it may cause confusion with emphasis.scaleSize, which is the enlarging animation when emphasizing. You are welcomed to suggest a new 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.
scalePercent
itemStyle.radius
itemStyle.scalePercent
(preferred)
also it would be an itemStyle.scaleSize
. but I think (not 100% sure yet =), i will lose the ability to use nonlinear functions. because percentages are calculated on the lib side in this case
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 would personnally perfer itemStyle.radiusPercent
to avoid using scaleXXX
, but not with very strong preference. If only itemStyle.xxx
is used but no series.xxx
, I think we should skip the callback function.
In ECharts, for options support both value and percentage form (like radius, we usually use strings like '30%'
to represent percentage. But since we don't support number type in our case, I think a 0-1 number type is acceptable.
test/pie-itemRadiusScale.html
Outdated
|
||
/** example #2: add pie with the same angles, | ||
* but full radius and semitransparent background */ | ||
createBackgroundSeries([1, 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.
I would suggest providing two new options series-pie.backgroundStyle
and series-pie.backgroundStyle
like series-bar.showBackground and series-bar.backgroundStyle, where series-pie.backgroundStyle.color
can be a callback function so that developers can set the background according to the piece color.
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.
You make make a seperate PR (or just in this if you like) for the background related feature because it also benifits the roseType pies.
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.
Intuitively, it seems that the background style in the series root will be applied to the entire pie. it's not obvious that the function is called on every element. If I'm trying to intuitively understand property names, then it seems more logical for me to put the style of the elements into itemStyle. Like series-pie.itemStyle.backgroundStyle.color
.
But this entry series-pie.backgroundStyle.opacity
with a numeric type rather than a function seems clear to me. This tells me that transparency will work the same on all elements
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.
Yes, I would slighter prefer to callback than under itemStyle
because users don't have to write multiple times. Another option could be, series-pie.backgroundStyle.color
has a default value 'auto'
meaning use color of each piece. series-pie.colorBy is 'series'
by default, so it's intuitively OK for the background to use the color of each piece.
Thanks for your time and so high evaluation of my work. This feature is called "cross selection" or "cross filtering" in Power BI. Using one vertical bar in some chart for cross selection is just a special case. There is almost no limitation about source of the filter. There may be an item from another pie chart (with different grouping settings), or rows/columns/cells from a table or any other custom component, or even selected cities/regions on the map. The targets of cross filtering are also may be different. In addition to pies, it can be columns (vertical or horizontal) or “stacked” areas on XY charts. There is some short youtube tutorial about cross filtering in powerbi with demo. Also, employees in my company are often used such feature in PBI. On the next screenshot you can see a fragment from the real dashboard in PBI, where i selected few rows within the table |
Thanks for you explanation! This sounds reasonable and I'm looking forward to it! |
radiusPercent now supports number format in addition to function. test file updated.
@Ovilia, I added my solution for background option. It's based on |
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 your update. I think we are quite close to the final edition!
@@ -299,6 +318,11 @@ class PieSeriesModel extends SeriesModel<PieSeriesOption> { | |||
opacity: 1 | |||
}, | |||
|
|||
showBackground: false, | |||
backgroundStyle: { | |||
opacity: 1 |
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.
Setting opacity: 1
by default is confusing and unconvenient. How about make it 0.25 by default?
const animationModel = seriesModel.isAnimationEnabled() ? seriesModel : null; | ||
const drawBackground = seriesModel.get('showBackground', true); | ||
const basicOuterRadius = getBasicPieLayout(seriesModel, api).r; | ||
const bgStyle = seriesModel.getModel('backgroundStyle').getItemStyle(); |
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 data.backgroundStyle
is not supported, how should developers set each pie piece to be a different color other than the default colors? I would suggest supporting data.backgroundStyle
but not data.showBackground
(only series.showBackground
is supported).
Please merge from the latest |
Brief Information
This pull request is in the type of:
What does this PR do?
itemRadiusScale
function added for pie series to control the radius of each itemFixed issues
#19020
#15674
Details
Before: What was the problem?
There was no easy option for pie charts to control the radius of items. In current version I can control either the whole pie radius or use some autocalculations with
roseType
option. May be, i can build many pies with single items and different start/end angles and personal pie radius, but this is mean also a lot of code around the lib every time. All this are not suitable for our tasks. Earlier we used dashboards in PowerBI and the same feature in PBI looked like:After: How does it behave after the fixing?
The solution is very similar with scatter charts and its
symbolSize
option. Now (with this PR) i can pass custom functionitemRadiusScale
, which returns value from 0 to 1. That value is used within pie component to calculate outer radius for each item. In simple words, we now have a third dimension for each item, which represents the size from center to the outer circle, almost like a scatter has x and y coordinates and a personal radius.If I provide custom non-linear function (see test file), I will get the same behaviour as in PBI. Like with logarithmic type Y-axis on XY charts, I can increase small numbers to see data more clearly.
Also, few calculated values (sum, maxValue) passed into
getDataParams
callback to make tooltip calculations easier. Not sure about necessary ofmaxValue
, butsum
is really helpful to calculate percents in formatters. And, when I disable some series on a legend, percents still calculated as I expected (it would be a pain without thesum
option).Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
test/pie-itemRadiusScale.html
here some content from the test file
Desktop.2023.12.24.-.02.04.56.05.mp4
Others
Merging options
Other information
This is my first PR here. Sorry, if i violated the rules (=