Scatter Chart with new api approach

[yang-wei]

I am experimenting with scatter chart and hope to discuss something with you before I commit. After reading some of the close issues/pr especially #9 , I really like the api approach like this.

<LineChart width={400} height={200} >
  <DataSeries data={data} label={label} color={red} />
  <DataSeries data={data} label={label} color={blue} />
  <DataSeries data={data} label={label} color={green} />
</LineChart>

For me this is more readable and user can fill in the color easily. So is that ok to open a pr with this approach of api design or you think it's better to keep current style?

Ya I also notice that we are facing component lifecycle issue if we depend d3.svg to render axes. Hope to come out with pure-react-rendering during my experiment.

[esbullington]

Hey @yang-wei that sounds great! I've thought a lot about the two different API approaches discussed in #9 and here's what I'd like to do: I'd like to offer both. So you'd create both the LineChart and the DataSeries components as suggested above (as described in #9, and as you probably already know, you'd access the nested DataSeries using this.props.children). Only instead of calling it LineChart, call it LineChartContainer. Then export both of them.

Finally, create a simple LineChart component that will just use the two components above like so:

var LineChart = React.createClass({
  ...
  render: function() {

    <LineChartContainer width={this.props.width} etc={etc} >
      {Object.keys(this.props.data).map(function(series) {
        return <DataSeries data={this.props.data[series] label={series} etc={etc} />;
      }
    </LineChartContainer>
  }
});

exports.LineChart = LineChart;

This way, people can continue to use the simple, single-component chart for quick charts, or charts that don't need much customization. But if they want to customize the data series, they can access them using the nested DataSeries API.

Essentially, we'll adopt the nested DataSeries API described in #9 but put a small abstraction over it so we can continue to offer the current API. It's not that I'm worried about breaking compatibility at this point (we're pre-beta, after all), although that is a nice plus. It's more that I want to make this a very easy library for people to use when they just need a quick chart. And if/when they need to do more customized work, the DataSeries API will be available to them.

What do you think?

PS: Making pure React axes that are compatible with the existing d3 utilities we are using will be a pretty big job -- I've already looked closely into it.

[esbullington]

@yang-wei By the way, just so we don't overlap each other's work, I wanted to mention my plans for today: I'm going to try to hit the following three things:

1. Factor out axes components to 1 single common component
2. Implement nested data series API as discussed above in LineChart and possibly AreaChart (stacked area)
3. Possibly implement legend as discussed in Legends / Labels for Line Charts? #13

But I'll leave the scatter chart to you. Feel free to give me your input/opinions on my proposed API described in the comment above.

One small issue/error I've made: I've forgotten to put 'use strict' at the top of a lot of the files, but I definitely want to use strict mode. So if you could put that on any new charts, that would be great.

[yang-wei]

Good one @esbullington !

It's more that I want to make this a very easy library for people to use when they just need a quick chart.

Yes I think supporting 2 ways and legend(I think this is important!) will make this library greater and the API you proposed lgtm =)

So I am going to implement Scatter Chart component which support both API first. After axes(common component) and legend are done, I will add them.

[esbullington]

Great @yang-wei , that sounds perfect! I'm working on the common axes right now. If you have time for it, I'll submit the PR and let you review it first. Otherwise, I'll go ahead and merge it.

One other thing I've noticed. In an effort to make things easy for users, I've been using props.color for the SVG stroke property (basically, I was just wanting to describe the axis color). But on retrospect, I think this was probably a poor idea on my part (despite my good intentions). The terms should be harmonized between SVG and this react-d3, just like they are harmonized between d3 and SVG. This way, users of this library can depend on the terminology used in d3 and SVG docs and tutorials.

The only change I'm making to the official SVG terms is using camel case in JS and JSX for SVG properties such as stroke-width (i.e. stroke-width -> strokeWidth), since JSX doesn't like the hyphenated properties.

I'll be updating the docs and code to reflect this change.

[yang-wei]

@esbullington I haven't figure out a clean way to support both api so I'd just open a pull request of scatter chart which support the API we are providing - passing all data through an object

<ScatterChart data={allData} />

I saw your issue for next version (0.2.0) published, so I think it would be nice to push this up first. Anyway I will be working on it once I have time. Cheers !

[esbullington]

Hey @yang-wei since you finished the scatter chart and we merged it, I'm going to close this issue now. If you have any additional questions or comments, please feel free to file another issue.

Thanks again for all your help!