Skip to content
This repository was archived by the owner on Sep 17, 2021. It is now read-only.

Fix pie chart centering #138

Merged
merged 2 commits into from
Apr 12, 2017
Merged

Conversation

dgladkov
Copy link
Contributor

  • I have signed the CLA
  • Fix NaN x/y values if options prop is supplied but does not contain margins
  • Fix centering - before this change centering applied two times and center option actually meant "x and y shift from the center" instead of "x and y position of the actual center".
  • Fix centering if only one data object is provided. Center option was ignored inthis.props.data.length === 1 codepath.
  • Do not create actual pie chart object if we have only 1 data object because we render Circle instead.

@dgladkov dgladkov changed the title Single item pie center Fix pie chart centering Apr 11, 2017
@@ -84,13 +84,7 @@ export default class PieChart extends Component {
R = (R || (this.props.options && this.props.options.R))
R = (R || radius)

let chart = Pie({
center: this.props.center || (this.props.options && this.props.options.center) || [0,0] ,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have center set to [0, 0] by default.

slices = chart.curves.map( (c, i) => {
let fill = (c.item.color && Colors.string(c.item.color)) || this.color(i)
let stroke = typeof fill === 'string' ? fill : Colors.darkenColor(fill)
return (
<G key={ i } x={x} y={y}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here actual centering was applied.

Copy link
Contributor

@marzolfb marzolfb left a comment

Choose a reason for hiding this comment

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

Thanks a lot for cleaning up several problems here - much appreciated!

@marzolfb marzolfb merged commit 1c34ca4 into capitalone:master Apr 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants