esbullington / react-d3

Modular React charts made with d3.js
https://reactiva.github.io/react-d3-website/
MIT License
1.75k stars 179 forks source link

Thoughts on `LineChart.props.color`? #14

Closed songgao closed 9 years ago

songgao commented 9 years ago

Currently LineChart.props.color is a function that takes in an argument being the index of the data series being drawn. However, it's hard to associate the index with keys (seriesName). This way, there doesn't seem to be a straightforward way to specify color for each data series explicitly. What do you think of letting the function accept keys (seriesName)?

A relevant idea is that, could we accept multiple data types as data, color, and legends (as proposed in #13)? Sometimes seriesName might only make sense within the react-d3, but the user may not care. If the user can feed in arrays, and associate color/legends with indices, it might be a good alternative interface?

esbullington commented 9 years ago

Let me think about this one. For right now, users can set specific palettes for data series, which is enough for many use cases.

But you've got some good points about the API, and whether we should even expose seriesName to the end user.

songgao commented 9 years ago

I'm thinking in comparison with plot() and its friends in Matlab or Python's matplotlib. Web is certainly more capable than those and shouldn't be constrained by traditions and assumptions from those tools, but I think we can learn from their API design :-)

esbullington commented 9 years ago

Still thinking about this one. @yang-wei what do you think? Should we move our data format to a multidimensional array instead of a series name mapped to data series array? If we do move, then that means users will have to manually a series name for each nested array. Hmm, I don't know.

If you look at how the Vega visualization grammar does this, they have an array of data series objects, each with a name property and values property. If we change, this is probably the way to go.

If we keep the format as is, then @songgao is right: we should probably fix the color function to take seriesName instead of index.

yang-wei commented 9 years ago

@esbullington

we should probably fix the color function to take seriesName instead of index

Right :+1:

I think keeping the current api is good for now. However, the ideal approach will be additional data support like proposed in #24.

<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>

But making this changes will require some works and time. Hopefully this can be made in v0.3.0 or v0.4.0.

Since we want to let users plot data as fast as possible, we should keep next publish simple. The easiest way to provide immediate support for optional color's feature I can think of is add another optional props like below.

var lineData = { series1: [...], series2: [...], series3: [...] }
var lineColor = { series1: "red", series2: "green", series3: "blue" };
<LineChart data={lineData} colors={lineColor} />
esbullington commented 9 years ago

Hey @songgao we moved to an array format for each data series, so you can pass in index-based palette functions now for the series. Please give it a try if you get a chance and let me know how it's working.

I'm closing this for now, please feel free to open another issue if you can think of one. Thanks!