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

Add initial victory-legend #116

Merged
merged 4 commits into from
Sep 21, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions demo/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from "react";
import ReactDOM from "react-dom";
import AnimationDemo from "./victory-animation/demo";
import LabelDemo from "./victory-label/demo";
import LegendDemo from "./victory-legend/demo";
import { Router, Route, Link, hashHistory } from "react-router";

const content = document.getElementById("content");
Expand All @@ -19,6 +20,7 @@ const App = React.createClass({
<ul>
<li><Link to="/animation">Victory Animation Demo</Link></li>
<li><Link to="/label">Victory Label Demo</Link></li>
<li><Link to="/legend">Victory Legend</Link></li>
</ul>
{this.props.children}
</div>
Expand All @@ -31,6 +33,7 @@ ReactDOM.render((
<Route path="/" component={App}>
<Route path="animation" component={AnimationDemo}/>
<Route path="label" component={LabelDemo}/>
<Route path="legend" component={LegendDemo}/>
</Route>
</Router>
), content);
71 changes: 71 additions & 0 deletions demo/victory-legend/demo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
import React from "react";
import { VictoryLegend } from "../../src/index";

export default class App extends React.Component {
render() {
const svgStyle = { border: "1px solid #ccc", padding: 20 };
const legendSize = {
width: 500,
height: 300
};
const data = [{
name: "Series 1",
label: {
fontSize: 10
},
symbol: {
type: "circle"
}
}, {
name: "Long Series Name",
label: {
fontSize: 12
},
symbol: {
type: "triangleUp",
style: {
fill: "blue"
}
}
}, {
name: "Series 3",
label: {
fontSize: 14
},
symbol: {
type: "diamond",
style: {
fill: "pink"
}
}
}, {
name: "Series 4",
label: {
fontSize: 16
},
symbol: {
type: "plus"
}
}, {
name: "Series 5",
label: {
fontSize: 18
},
symbol: {
type: "star",
style: {
fill: "red"
}
}
}];

return (
<div className="demo">
<VictoryLegend {...legendSize} data={data} style={svgStyle} />
<svg {...legendSize} style={svgStyle}>
<VictoryLegend {...legendSize} orientation="horizontal" data={data} standalone={false}/>
</svg>
</div>
);
}
}
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@ export { default as VictoryTransition } from "./victory-transition/victory-trans
export { default as VictorySharedEvents } from "./victory-shared-events/victory-shared-events";
export { default as VictoryContainer } from "./victory-container/victory-container";
export { default as VictoryTheme } from "./victory-theme/victory-theme";
export { default as VictoryLegend } from "./victory-legend/victory-legend";
186 changes: 186 additions & 0 deletions src/victory-legend/victory-legend.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
/*global document:false */

import React, { PropTypes } from "react";
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
Copy link
Contributor

Choose a reason for hiding this comment

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

If you merge master you can use the point component victory-primitives/point


const defaultFontStyle = {
fontSize: 14,
fontFamily: "'Gill Sans', 'Gill Sans MT', 'Ser­avek', 'Trebuchet MS', sans-serif",
color: "#252525",
backgroundColor: "#d9d9d9",
stroke: "transparent"
};

const defaultLegendStyle = {
height: 200,
width: 300,
padding: 20
};

const defaultLegendData = [{
name: "Series 1",
symbol: {
type: "circle",
style: {
fill: "red"
}
}
}, {
name: "Series 2",
symbol: {
type: "diamond",
style: {
fill: "blue"
}
}
}];

export default class VictoryLegend extends React.Component {
static propTypes = {
x: PropTypes.number.isRequired,
y: PropTypes.number.isRequired,
height: PropTypes.number.isRequired,
width: PropTypes.number.isRequired,
orientation: PropTypes.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add an overall padding prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Still missing a padding prop in the validation.

data: PropTypes.arrayOf(
PropTypes.shape({
name: PropTypes.string.isRequired,
label: PropTypes.object,
symbol: PropTypes.object
})
),
columnItemSpacing: PropTypes.number,
rowItemSpacing: PropTypes.number,
groupComponent: PropTypes.element,
standalone: PropTypes.bool,
style: PropTypes.object
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the style object have namespaces so you can style the text, symbol, and container separately? I would suggest parent, data, labels like a lot of the other components use.

Copy link
Contributor

@boygirl boygirl Sep 5, 2016

Choose a reason for hiding this comment

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

Legends should be able to work with theme, but I don't mind if you save this for v2

Copy link
Contributor

Choose a reason for hiding this comment

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

Legends should work with Victory's event system. This can be v2 work if you prefer, and I'm happy to implement it if you're having trouble with it. If you want to give it a shot, I've written about the pattern I'm using to implement events here https://github.com/FormidableLabs/builder-victory-component/blob/master/dev/CONTRIBUTING.md#events

};

static defaultProps = {
x: 0,
y: 0,
height: 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

width and height don't seem to be used at all. Instead they are coming from default legend style?

width: 150,
orientation: "vertical",
columnItemSpacing: 10,
rowItemSpacing: 8,
symbolComponent: <Point/>,
labelComponent: <VictoryLabel/>,
containerComponent: <VictoryContainer/>,
groupComponent: <g/>,
standalone: true
};

getFontStyles(props) {
return merge({}, defaultFontStyle, props.label);
Copy link
Contributor

Choose a reason for hiding this comment

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

if these are flat objects, prefer defaults to merge for perf reasons

}

getLegendStyle(props) {
return {
style: merge({}, defaultLegendStyle, props.style)
};
}

// it should be changed to approximateTextSize
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please do use approximateTextSize, it works well. It is merged into master now.

getLabelBbox(text, font) {
const body = document.body;
const container = document.createElement("svg");
const label = document.createElement("text");

label.innerText = text;
label.setAttribute("style",
`fill: ${font.color};
font-size: ${font.fontSize}px;
font-family: ${font.fontFamily};
background-color: ${font.backgroundColor};
stroke: ${font.stroke};
top: -10000px;
position: absolute;`
);
body.appendChild(container);
container.appendChild(label);

const bBox = label.getBoundingClientRect();
body.removeChild(container);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have concerns about this pattern. Everything will need to be extendable to the React Native version of Victory


return bBox;
}

renderContainer(props, group) {
return React.cloneElement(
props.containerComponent,
this.getLegendStyle(props),
group
);
}

renderGroup(children) {
return React.cloneElement(
this.props.groupComponent,
{ role: "presentation" },
children
);
}

render() {
let yPadding = 0;
let xPadding = 0;
const props = this.props;
const itemXPosition = [];
const symbolComponents = [];
const labelComponents = [];
const { symbolComponent, labelComponent } = props;
const data = isEmpty(props.data) ? defaultLegendData : props.data;

data.forEach((series, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For perf reasons we've been replacing array method that loop across data with length cached for loops

const font = this.getFontStyles(series);
const symbolSize = font.fontSize;
const hPadding = symbolSize * 0.87;
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic numbers...

const yPos = props.y + index * yPadding;
const labelSymbolPadding = symbolSize * 1.5 + props.columnItemSpacing;

if (props.orientation === "horizontal") {
itemXPosition.push(xPadding);
Copy link
Contributor

Choose a reason for hiding this comment

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

assign by array index rather than .push for perf.

xPadding += labelSymbolPadding + this.getLabelBbox(series.name, font).width;
} else {
yPadding = symbolSize + props.rowItemSpacing;
}

const xLabelPos = (props.orientation !== "horizontal" ?
props.x : itemXPosition[index]) + hPadding;
const xSymbolPos = props.orientation !== "horizontal" ?
props.x : itemXPosition[index];

const symbolProps = assign({
key: `symbol-${index}`
}, {
x: xSymbolPos,
y: yPos,
size: series.symbol.size || symbolSize / 2.5,
symbol: series.symbol.type
}, series.symbol);

const labelProps = assign({
key: `label-${index}`
}, {
x: xLabelPos,
y: yPos,
style: font,
text: series.name
}, series.label);

symbolComponents[index] = React.cloneElement(
Copy link
Contributor

@boygirl boygirl Sep 5, 2016

Choose a reason for hiding this comment

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

If you move all the stuff that is actually rendered into a new method it will be much easier to write the native version. I wrote up a little development guide with examples. You can see the patterns I've been using here https://github.com/FormidableLabs/builder-victory-component/blob/master/dev/CONTRIBUTING.md#flexibility

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please clarify it.

symbolComponent, assign({}, symbolProps)
);
labelComponents[index] = React.cloneElement(
labelComponent, assign({}, labelProps)
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary assign

);
});

const group = this.renderGroup([...symbolComponents, ...labelComponents]);
return props.standalone ? this.renderContainer(props, group) : group;
}
}
Loading