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

Include option for padAngle on RadialChart #920

Merged
merged 7 commits into from
Aug 21, 2018
Merged

Conversation

dvnrsn
Copy link
Contributor

@dvnrsn dvnrsn commented Aug 21, 2018

Currently there are no means to add padding between arcs on a RadialChart. This PR seeks to enable such capability.

@CLAassistant
Copy link

CLAassistant commented Aug 21, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@mcnuttandrew mcnuttandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice add! I like what you got going here. I have a few requests:

  • Will you include a showcase example? That way it can be injected into the docs to demonstrate to what padAngle means?
  • For consistancy will you add this to the sunburst as well?
  • Will you include some pics so we know that these changes work?

@@ -180,6 +182,7 @@ class ArcSeries extends AbstractSeries {
endAngle: angle(row)
};
const arcedData = arcBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe make this a one liner arcBuilder().padAngle(padAngle)

@@ -191,6 +191,7 @@ RadialChart.propTypes = {
).isRequired,
getAngle: PropTypes.func,
getAngle0: PropTypes.func,
padAngle: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe set the default padAngle on here too?

@mcnuttandrew
Copy link
Contributor

It looks like the CLA is mad at you. I think you can get around this by squashing this whole branch into a single commit

@dvnrsn
Copy link
Contributor Author

dvnrsn commented Aug 21, 2018

Updated email but can't get CLA to play nicely. Going to refork and create new PR...

@mcnuttandrew
Copy link
Contributor

it also looks like lint is failing!

@dvnrsn
Copy link
Contributor Author

dvnrsn commented Aug 21, 2018

padAngle={0}
screen shot 2018-08-21 at 10 50 14
padAngle={0.03}
screen shot 2018-08-21 at 10 50 49

@dvnrsn
Copy link
Contributor Author

dvnrsn commented Aug 21, 2018

Is showcase code rendered somewhere? Is this the place where I should add padAngle? @mcnuttandrew

Copy link
Contributor

@mcnuttandrew mcnuttandrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also include an example of using the padAngle as a function somewhere, like maybe on the Sunburst with tooltips showcase example?

@@ -67,6 +67,10 @@ SVG paths (which is what the arc series is made up of) have numerous manipulable
Type: `string`
The className to be added to an individual arc in the series.

#### padAngle (optional)
Type: `number|function`
The padding to be applied between arcs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think including some text here like, "see above donut chart for an example of a padded angle" would be fine

@mcnuttandrew
Copy link
Contributor

Alright looks good! Stamp stamp stamp!!!!

@mcnuttandrew mcnuttandrew merged commit 17d7969 into uber:master Aug 21, 2018
ayarcohaila pushed a commit to ayarcohaila/react-vis that referenced this pull request Jun 30, 2021
* add padAngle functionality

* remove trailing comma

* include padAngle in sunburst

* include padangle in sunburst docs

* include padAngle in simple donut chart showcase

* includ clarification text to radial-chart.md

* include padAngle as function in sunburstWithTooltips
ayarcohaila added a commit to ayarcohaila/react-vis that referenced this pull request May 30, 2023
* add padAngle functionality

* remove trailing comma

* include padAngle in sunburst

* include padangle in sunburst docs

* include padAngle in simple donut chart showcase

* includ clarification text to radial-chart.md

* include padAngle as function in sunburstWithTooltips
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants