Skip to content
This repository has been archived by the owner on May 18, 2019. It is now read-only.

Common axes #27

Merged
merged 1 commit into from
Jan 22, 2015
Merged

Common axes #27

merged 1 commit into from
Jan 22, 2015

Conversation

esbullington
Copy link
Owner

No description provided.

@esbullington
Copy link
Owner Author

@yang-wei Did you want to review this PR before I merge it? Otherwise, no worries.

@esbullington esbullington mentioned this pull request Jan 22, 2015
@esbullington
Copy link
Owner Author

Thanks @yang-wei You bring up a good point, and one that I looked into closely myself. Concern about refs overwriting each other was why I namespaced the refs before. However, according to the React documentation, refs should be per instance, so I decided not to worry about it for the time being.

You make a very good point that if/when we start doing data frame transitions, we'll want a static reference since we'll be instantiating new chart components with each new frame, and the instance types of two different charts could overlap. I had not considered this issue.

On the other hand, having to create a unique ref for each chart type that would be included in each axis component (not to mention the unique CSS class names) is a pretty ugly hack -- and the only elegant way around this I can see is to create pure React axes.

But pure React axes that are compatible with d3 utilities is a big job, and I'd rather spend energy making new charts and cleaning up the code base at this point. I'll aside a few weekends in February to make the axes (if it takes as long as I think it will).

Also, before we even think about adding animations between data frames, we'll need to build some kind of chart state component mixin. I'm looking at an immutable cursor for chart state, since that will allow us to easily fast forward and rewind the data states (e.g., charts on different dates). I'll open an issue when we're at that point to get your input and ideas.

But let's not worry about these things until v0.3.0 at least.

For upcoming v0.2.0 release, I'd like to mostly limit ourselves to the things I listed in #28, although I'd be happy to add one more chart to that list if you have something else in mind.

So while you're absolutely correct that transitions between chart states will cause problems with these refs, I'm going to leave them as is for now. If we don't have pure React axes by 0.3.0, I'll make the changes you suggest.

Factor out xAxis and xAxis components from linechart.js, areachart.js,
and barchart.js to common.js, preparing the library for whenever the
chart axes are switched from d3-generated svg to pure React-generated
axis svg.
esbullington added a commit that referenced this pull request Jan 22, 2015
@esbullington esbullington merged commit 5713cfb into master Jan 22, 2015
@yang-wei
Copy link
Collaborator

Ok great ! Axes rendered using react will be a really cool thing. So I will merge latest master branch and start working on scatter chart ssoon :-)

@esbullington esbullington deleted the common_axes branch January 23, 2015 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants