VictoryChart: adds an option for adding background for chart area #1558
Conversation
wsparsons
changed the title
wsparsons
changed the title
…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
wsparsons
force-pushed
the
task/add-chart-background-primitive
branch
from
dbbb9c6
to
ce270ae
Compare
| polar: props.polar, | ||
| scale: calculatedProps.scale, | ||
| style: props.style.background, | ||
| width: width |
There was a problem hiding this comment.
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.
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.
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.
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.
The main victory package should also reexport the new Background primitive
| width | ||
| }; | ||
|
|
||
| return React.cloneElement(backgroundElement, backgroundProps); |
There was a problem hiding this comment.
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)
…Background from Victory package
VictoryChart
Backgroundprimitive componentVictoryChartto allow an option for adding background for just the chart areaVictoryChartPropsinterfaceVictoryChartto accept abackgroundpropVictoryChartstorybook to visually check that the background color is rendered correctlyStorybook Screenshots