Add initial victory-legend #116
Conversation
| container.appendChild(label); | ||
|
|
||
| const bBox = label.getBoundingClientRect(); | ||
| body.removeChild(container); |
There was a problem hiding this comment.
I have concerns about this pattern. Everything will need to be extendable to the React Native version of Victory
|
@janesh-travolta This is generally looking good! Here's my high-level feedback: Anything that renders
It should be possible to specify everything about the style of a symbol and label
|
| import { merge, assign, isEmpty } from "lodash"; | ||
| import VictoryLabel from "../victory-label/victory-label"; | ||
| import VictoryContainer from "../victory-container/victory-container"; | ||
| import Point from "../point/point"; // it should be replaced |
There was a problem hiding this comment.
If you merge master you can use the point component victory-primitives/point
There was a problem hiding this comment.
Overall there is some confusion about how theme / props / fallback props should operate together. I think I would like to open a branch for you to merge this work into rather than master so that we can collaborate more directly on getting this more consistent with other Victory components.
| static defaultProps = { | ||
| x: 0, | ||
| y: 0, | ||
| height: 100, |
There was a problem hiding this comment.
width and height don't seem to be used at all. Instead they are coming from default legend style?
| }; | ||
|
|
||
| getLabelStyles(props) { | ||
| return merge({}, fallbackProps.style.labels, props.label); |
There was a problem hiding this comment.
props.label is not defined
| const { dataComponent, labelComponent } = props; | ||
| const data = isEmpty(props.data) ? defaultLegendData : props.data; | ||
|
|
||
| data.forEach((series, i) => { |
There was a problem hiding this comment.
This should be a length cached for loop for perf reasons
| ); | ||
| }); | ||
|
|
||
| const group = this.renderGroup([...dataComponents, ...labelComponents]); |
There was a problem hiding this comment.
I would prefer if calculating all the dataComponents and labelComponents was moved outside of the render method like:
renderLegend(props) {
...
return [...dataComponents, ...labelComponents];
}
render() {
// do all your style / theme / fallback prop merging here
const group = this.renderGroup(this.renderLegend(reconciledProps));
return props.standalone ? this.renderContainer(props, group) : group;
}
| dataComponent, assign({}, symbolProps) | ||
| ); | ||
| labelComponents[i] = React.cloneElement( | ||
| labelComponent, assign({}, labelProps) |
| }, assign({}, fallbackProps.style.labels, props.style.labels, series.label)); | ||
|
|
||
| dataComponents[i] = React.cloneElement( | ||
| dataComponent, assign({}, symbolProps) |
| data.forEach((series, i) => { | ||
| const font = this.getLabelStyles(series); | ||
| const symbolSize = font.fontSize; | ||
| const hPadding = symbolSize * 0.87; |
|
@janesh-travolta I'm going to merge this into the legend branch for ease of collaboration |
No description provided.