Skip to content

Fix issue #1120#1129

Merged
narinluangrath merged 2 commits intomasterfrom
feature/issue-1120
Oct 5, 2018
Merged

Fix issue #1120#1129
narinluangrath merged 2 commits intomasterfrom
feature/issue-1120

Conversation

@narinluangrath
Copy link
Copy Markdown
Contributor

  • Add getPath prop to victory bar
  • Add stories to storybook demonstrating usage
  • For non-polar charts, getPath expects a function ({ x0, x1, y0, y1}) -> (string describing svg path)
  • For polar charts, getPath expects a function ({ datum, start, end, r1, r2 }) -> (string describing svg path), where datum is an object of shape {x, y}, start/end are angles derived from styles.width, and r1/r2 are the radii (top/bottom) of each polar bar

- Add getPath prop to victory bar
- For non-polar charts, getPath expects a function ({ x0, x1, y0, y1}) -> string describing svg path
- For polar charts, getPath expects a function ({ datum, start, end, r1, r2 }) -> string describing svg path, where  is an object of shape {x, y},  are angles derived from styles.width, and  are the radii (top/bottom) of each polar bar
- Add stories to storybook demonstrating usage
@narinluangrath narinluangrath requested a review from boygirl October 1, 2018 03:45
Copy link
Copy Markdown
Contributor

@boygirl boygirl left a comment

Choose a reason for hiding this comment

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

I would much prefer getPath to take a single argument props and have the same signature for polar and standard bars. To make it easier for folks, the props that we pass in can include all the pre-calculated values like width, startAngle and endAngle

@narinluangrath
Copy link
Copy Markdown
Contributor Author

Yeah I was definitely not sure about what variables to include in the getPath function. Happy to make that change! Just to clarify, when you say the getPath function takes an argument props, you are specifically referring to this.props in the <Bar> component? You're not using props as a generic variable name?

I've changed the code so that getPath for non-polar bar charts has signature props -> svgPathString where props is a copy of this.props with additional fields x0, x1, y0, y1. The same is true for polar charts, except the additional fields are startAngle, endAngle, r1, r2.

FYI: I've added some stories to test the getPath prop but I'm really bad/slow at drawing svgs. Feel free to replace/remove that code.

Copy link
Copy Markdown
Contributor

@boygirl boygirl left a comment

Choose a reason for hiding this comment

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

Yes! This is much better! Good to merge :)

@boygirl
Copy link
Copy Markdown
Contributor

boygirl commented Oct 5, 2018

Also, your polar shooting stars look rad

@narinluangrath narinluangrath merged commit e271985 into master Oct 5, 2018
@narinluangrath narinluangrath deleted the feature/issue-1120 branch October 5, 2018 02:56
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.

2 participants