Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

added radius prop to VictoryPie per issue #910 #182

Merged
merged 5 commits into from
Jun 23, 2018
Merged

Conversation

BotScutters
Copy link
Contributor

@BotScutters BotScutters commented Jun 14, 2018

Added support for radius prop to Victory-Pie. If radius prop is not present, radius is calculated from width, height, and padding as before.
-Added radius prop to propTypes in victory-pie.js
-Added check to getRadius in helper-methods.js

Closes FormidableLabs/victory#910

@boygirl
Copy link
Contributor

boygirl commented Jun 20, 2018

Please add an origin prop that will be an object like { x: number, y: number }. When this prop is provided it will be used rather than calculated from width / height / padding.

@BotScutters BotScutters reopened this Jun 23, 2018
@BotScutters
Copy link
Contributor Author

Added support for origin prop to Victory-Pie. If origin prop is not present, origin is calculated from width, height, and padding as before.
-Added origin prop to propTypes in victory-pie.js as an array containing x and y, with x and y able to be either numbers or functions. Should they actually just be numbers and not allow functions?
-Added check in helper-methods.js for whether or not origin prop is provided

Closes FormidableLabs/victory#910

@boygirl
Copy link
Contributor

boygirl commented Jun 23, 2018

The values for origin.x and origin.y should be restricted to non-negative numbers. Functions don’t make sense here.

@@ -102,6 +102,16 @@ class VictoryPie extends React.Component {
labelRadius: PropTypes.oneOfType([ CustomPropTypes.nonNegative, PropTypes.func ]),
labels: PropTypes.oneOfType([ PropTypes.func, PropTypes.array ]),
name: PropTypes.string,
origin: PropTypes.arrayOf(PropTypes.shape({
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be an array of objects. Just a single one.

CustomPropTypes.allOfType([CustomPropTypes.integer, CustomPropTypes.nonNegative])
]),
y: PropTypes.oneOfType([
PropTypes.func,
Copy link
Contributor

Choose a reason for hiding this comment

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

These can just be CustomPropTypes.nonNegative

…igin definition and removed offsetWidth and offsetHeight in helper-methods
Copy link
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.

Approved! Thank you for this change

@BotScutters BotScutters merged commit e5b1d44 into master Jun 23, 2018
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