Skip to content
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

[5.0] [Feature] sunburst series supports cornerRadius #13390

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

plainheart
Copy link
Member

@plainheart plainheart commented Oct 5, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Sunburst series supports corner radius in ECharts, please refer to #13123 & #13378 for more details.

Fixed issues

Details

image

Usage

Are there any API changes?

  • The API has been changed.

Added a new option to the sunburst series.

  • cornerRadius (can be a string/number/string array/number array)

Related test cases or examples to use the new APIs

NA.

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

#13378 - Pie series supports cornerRadius
fe0d94a - Treemap series supports borderRadius

@echarts-bot
Copy link

echarts-bot bot commented Oct 5, 2020

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

The pull request is marked to be PR: author is committer because you are a committer of this project.

Document changes are required in this PR. Please also make a PR to apache/incubator-echarts-doc for document changes. When the doc PR is merged, the maintainers will remove the PR: awaiting doc label.

@pissang
Copy link
Contributor

pissang commented Oct 6, 2020

The result is beautiful. Great job!

@pissang pissang merged commit 51a13cd into apache:next Oct 6, 2020
@echarts-bot
Copy link

echarts-bot bot commented Oct 6, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@100pah
Copy link
Member

100pah commented Oct 8, 2020

  • Do we name it as borderRaidus as CSS did or name it as cornerRadius?
    Currently we have borderColor borderWidth that the same as CSS naming.
    borderRaidus may be easy to remember a little. But cornerRadius seem more semantically correct.
  • Should cornerRadius be set in itemStyle as borderColor borderWidth did or set in the root of series? Probably it's better to follow the convention to put it in itemStyle?

@pissang @plainheart

@plainheart
Copy link
Member Author

plainheart commented Oct 8, 2020

Do we name it as borderRaidus as CSS did or name it as cornerRadius?

I thought about the naming at first.
Though I would like to name it after borderRadius to keep naming uniform, I think cornerRadius is more suitable for the sector shape and is more semantic. Moreover, I found most of chart libraries call it cornerRadius, such as amcharts, d3.
However, on the contrary, borderRadius is more corresponding with the naming of border in CSS and many options in ECharts are currently using this kind of naming.
I'm afraid it's hard for me to decide to use which one.

Should cornerRadius be set in itemStyle as borderColor borderWidth did or set in the root of series? Probably it's better to follow the convention to put it in itemStyle?

I think it's generally okay. Putting cornerRadius into itemStyle may enable users to control easily style of each item(sector).
That looks better.

@100pah

@pissang
Copy link
Contributor

pissang commented Oct 9, 2020

Though I would like to name it after borderRadius to keep naming uniform, I think cornerRadius is more suitable for the sector shape and is more semantic. Moreover, I found most of chart libraries call it cornerRadius, such as amcharts, d3.
However, on the contrary, borderRadius is more corresponding with the naming of border in CSS and many options in ECharts are currently using this kind of naming.

Almost the same concern. But I think a uniform naming is slightly better after struggling with my mind.

@plainheart Are you planning to do these two changes? I'm fine to do this work today.

@plainheart
Copy link
Member Author

@pissang No. I'm glad that you are going to help me do this. Thank you!

@plainheart plainheart deleted the feat/sunburst-cornerRadius branch May 2, 2021 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: author is committer PR: awaiting doc Document changes is required for this PR. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants