Several enhancements for line charts, title for all charts

[tarrencev]

No description provided.

[tarrencev]

Should the public/js/_.js above be dist/js/_js? Prob don't want to ignore the whole dist folder

[tarrencev]

I'm building an application using react-d3 which need these features. I think they would be a great addition in general though

[esbullington]

Hey, thanks for the PR! Everything is an easy pull except for the multiple data series, which I'd like to discuss with you a little if you don't mind. Also, I'd like to get your input on where to render the axes, since the DOM interactions between d3 and React are a little tricky (we can discuss this in #8).

But first, let me mention my overall goal that I'd like to work toward (at least eventually) for this library: I'd love to keep the mutable state down to a very minimum, and pass down any changes from the top component down using props. Now, that's not possible now since we're using d3's svg generation for the chart axes. But I'd love to get to a point where we could use React to generate the axes, and not worry about any of this lifecycle stuff because of how d3 interacts with React. But I'm well aware that duplicating d3's chart axes in React is a lot of work, which is why I'm kicking it down the road for now.

So for now, the multi data series: I'm glad you've addressed this issue. I've been wanting to support multiple data series but have been real busy at work the past week so I've not been able to move this forward. I've been wavering between the approach you took, which is to include the data series in the initial data, and taking the approach of leveraging React to support the multiple series, like this:

<LineChart width={400} height={200} >
  <DataSeries data={data} label={label} />
  <DataSeries data={data} label={label} />
  <DataSeries data={data} label={label} />
</LineChart>

Of course, instead of repeating the DataSeries, you'd usually just do:

arr.map(function(val, idx) {
  return   <DataSeries data={val.data} label={val.label} key={idx} />;
});

This is why I broke the DataSeries out into a separate React component in all the chart. This leverages React's flexibility and lets the user supply the data series from whatever form they are using to store their series.

That said, I think the approach you took might be more user friendly, particularly to users who aren't used to React. We could actually support both approaches, initially checking for a data prop in the LineChart initialization and then falling back on this.props.children if it's not there. But first, let me ask you and anyone else reading: is this API worth pursuing, or should we keep it simple and use just the approach in your PR:

 var data = {
    series1: [ { x: 0, y: 20 }, { x: 1, y: 30 }, { x: 2, y: 10 }, { x: 3, y: 5 }, { x: 4, y: 8 }, { x: 5, y: 15 }, { x: 6, y: 10 } ],
    series2: [ { x: 0, y: 8 }, { x: 1, y: 5 }, { x: 2, y: 20 }, { x: 3, y: 12 }, { x: 4, y: 4 }, { x: 5, y: 6 }, { x: 6, y: 2 } ],
    series3: [ { x: 0, y: 0 }, { x: 1, y: 5 }, { x: 2, y: 8 }, { x: 3, y: 2 }, { x: 4, y: 6 }, { x: 5, y: 4 }, { x: 6, y: 2 } ]
};

<LineChart data={data} width={400} height={200} />

What do you think? I'm leaning toward thinking my proposed nested DataSeries is probably not the best API to offer here.

[tarrencev]

Interesting, I hadn't put too much thought into the data structure tbh.

Looking at these two options, I actually prefer the one you've proposed, having each defined within the tags.

Like you said, this provides more flexibility of where the data is coming from. We won't require users to conform to the nested series schema and it would be a more elegant solution for single data series charts.

One thing I would advise against though, is trying to allow both. Maintaining both could turn out to be a pain down the road.

I would be happy to make the implementation changes for whatever we decide on.

On your note about state, I completely agree. The state I've added to LineChart is the internal dimensions calculated by the component which the component might update later (i.e. creating a responsive chart that adjusts to it's parent container). I think that is an appropriate use of state but would be happy to hear your thoughts. The line between props and state can be blurry.

[esbullington]

Having thought about and waited a bit to see if anyone else has input, let's go ahead and do multiple series using the nested <Dataseries/> (accessing them using this.props.children). It adds on a bit more boilerplate code, but it also let's us expose that component for customization, so we can eventually include options like animation there. (that brings up your other issue about styling, which is something I'm still thinking about and will respond to soon).

And it may -- just maybe -- turn out to be a more intuitive way for non-programmers who want to use a web chart library to reference and style multiple series.

So if you feel like it, it'd be great if you're willing to make that change to your multiple data series patch. Otherwise, if you want to just remove that particular patch from this PR and resubmit it, I'll go ahead and pull it in and then I'll do it myself later this week or this weekend.

(By the way, I know the example docs are a pain in the ass right now because of having to manually insert the newlines, so I really appreciate the fact that you went ahead and included an example with the multiple series in this PR. I'd like to find a way to move away from this set-up soon -- so it'd probably a good idea for me to check out how react-bootstrap is doing their documentation. If you have any alternate ideas on how to do documentation for a React project (that integrates live React examples), I'm definitely interested.)

[tarrencev]

Cool, I'll take care of making these changes

[tarrencev]

Working on the linechart changes. How should we pass the scaleX, scaleY, width, height, color, and point radius to each dataseries? The DataSeries element defined within the LineChart tags will only take data and title as props but the DataSeries component needs all these props to render. I think we will need to augment the DataSeries elements with the additional props inside of LineChart.

We could loop over {this.props.children} inside the LineChart component and add the props there but is there a better way?

[esbullington]

That's a tough question. But first of all, I do think we should definitely expose color (actually, the svg term fill is what I was using) and point radius, although we should set default values for those using getDefaultProps Many users will definitely want to control the colors of the series and the radius size of the data points, and if we're not using stylesheets, this is the obvious place to allow it.

But for behind-the-scenes things like scaleX, scaleY, width, and height, I can think of two alternatives. We definitely don't want to make users pass on all those props. So we either set up a global pubsub system -- something like Flux -- or we could look at using a cursor. I've used Flux-like patterns in a production app, and it's worked OK, although I think it's too much complexity for a project like this. I think a cursor may be a good compromise between a full Flux-like system and passing on all those props. Check out react-cursor. I've also been looking at omniscientjs.

Of course, we'd still need to pass on a cursor prop, but it would hide all the other state that would need to be passed on.

I think cursors may be a good approach. What do you think?

[esbullington]

I went ahead a merged your PR, much appreciated! If we end up deciding to use the DataSeries API, we're still early alpha and can change if we need to. Meanwhile, you made some needed fixes so I'm merging it as is.

(By the way, I did change one small thing, which was your .gitignore edit. We need to keep the dist/ directory checked in since I use a git submodule within that directory to generate the gh-pages documentation. I'm open to suggestions on how to better autogenerate that documentation but for now it's needed.)