Make xScale and yScale available in Chart context

[skeate]

Closes #21

[jameslaneconkling]

I had originally set it up this way, but ran into the following issues:

• props passed from parent to child were not available at render time. I implemented sharing of scales slightly differently, see this commit that removed it.
  • b/c of above, props couldn't be validated w/ propTypes
• there are cases where you might want to pass different scales to subcomponents, e.g. here, where the axes use one set of scales, and the grid lines use another. Or, e.g. a dual scale chart.

It could be that your approach (using context type, rather than yielding additional properties to child components) doesn't run into the first two issues, will test out. And I assume properties passed as context type can be overwritten by properties passed explicitly (need to check).

Assuming the above points aren't issues, I'll merge. Give me a sec to test it out.

[skeate]

Not entirely sure what you were doing before -- looks like you cloned the children and manually inserted the xScale and yScale? This uses the built-in contextTypes mechanism, so it shouldn't have any issues. I also changed the styleguide and was running that to test that it worked.

The second point shouldn't be an issue, as with my PR it will always prioritize (e.g.) this.props.xScale over this.context.xScale, if this.props.xScale exists:

const xScale = this.props.xScale || this.context.xScale;

[skeate]

The only downside I see is that it's possible to forget to pass in xScale or yScale, and you won't get a warning about it being required, since it no longer is truly required in any one place (since Chart doesn't need it if doing a pie chart, for example)

[skeate]

I think I might see what you are talking about in the first bullet, but I think it's still related to the whole cloneElement thing.

[skeate]

So I removed it entirely!

[jameslaneconkling]

yeah, the

React.Children.map(children, (child) => React.cloneElement(child, { ...otherProps });

is I think the recommended way to yield properties from a parent to a child (or at least based on this SO answer). But I think you're right, b/c when using contextType, those props are available at render.

I'll look the PR over one last time this morning and merge.

[skeate]

Missed one.

[skeate]

That SO post is old (in Javascript terms) -- there's a newer answer below that uses context.

[jameslaneconkling]

yeah, but I kind of agreed (at the time at least) w/ the "it's experimental and likely to break w/ future releases" warning comment.

Now, I'm less sure. I think they just want to make sure people don't abuse it.