-
Notifications
You must be signed in to change notification settings - Fork 525
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
VictoryChart: adds an option for adding background for chart area #1558
Conversation
…der the Background component if the 'background' prop exits on the style object
…exists on style object passed to VictoryChart
…o charts w/ 0 children
…prop for VictoryChart
…kground.js, clean up demo
dbbb9c6
to
ce270ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is working well, but I'd like to see the props on Background
cleaned up a bit, and put more responsibility on VictoryChart
for calculating the positioning props.
polar: props.polar, | ||
scale: calculatedProps.scale, | ||
style: props.style.background, | ||
width: width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's calculate x
/ cx
and y
/ cy
here, too
@@ -87,10 +87,18 @@ export default class VictoryChart extends React.Component { | |||
getNewChildren(props, childComponents, calculatedProps) { | |||
const children = getChildren(props, childComponents, calculatedProps); | |||
const getAnimationProps = Wrapper.getAnimationProps.bind(this); | |||
return children.map((child, index) => { | |||
const backgroundComponent = getBackgroundWithProps(props, calculatedProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this calculation into the props.style.background
conditional so we can skip it if we don't need it.
…s for horizontal component
…zontal), adds extra proptypes, exports Background from VictoryCore DefinitelyTyped types
…, add a chart with background to chart demos
style: props.style.background, | ||
x: xCoordinate, | ||
y: yCoordinate, | ||
width |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add scale
to the list of backgroundProps.
Background.propTypes = { | ||
...CommonProps.primitiveProps, | ||
circleComponent: PropTypes.element, | ||
cx: PropTypes.number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like cx and cy are never passed in.
…d prop getter is only invoked when it's going to be used
@@ -7,6 +7,7 @@ export { default as VictoryTheme } from "./victory-theme/victory-theme"; | |||
export { default as VictoryPortal } from "./victory-portal/victory-portal"; | |||
export { default as Portal } from "./victory-portal/portal"; | |||
export { default as Arc } from "./victory-primitives/arc"; | |||
export { default as Background } from "./victory-primitives/background"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main victory
package should also reexport the new Background
primitive
width | ||
}; | ||
|
||
return React.cloneElement(backgroundElement, backgroundProps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use lodash defaults
so that props set directly on the backgroundComponent
are not nuked by props from the parent.
Like imagine the following scenario where a user wants the background to span the whole height of the svg but be confined to the chart width:
<VictoryChart
height={300}
width={300}
style={{ background: { fill: "pink" } }}
backgroundComponent={<Background y={0} height={300} />
...
The y
value and height
directly on Background
should take prececence over what VictoryChart
is trying to set.
I think the props should be defaults({}, backgroundElement.props, backgroundProps)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great point
…Background from Victory package
VictoryChart
Background
primitive componentVictoryChart
to allow an option for adding background for just the chart areaVictoryChartProps
interfaceVictoryChart
to accept abackground
propVictoryChart
storybook to visually check that the background color is rendered correctlyStorybook Screenshots