heavysixer / d4

A friendly reusable charts DSL for D3
MIT License
432 stars 46 forks source link

Append features once, then reselect for data bind #27

Closed nsonnad closed 9 years ago

nsonnad commented 9 years ago

This PR uses the handy d4.appendOnce convention for new series and axes. It then does a data bind and reselects the elements to apply attributes. This prevents features from being drawn twice. You can see, for example, in the dynamic data example, that the axes are re-appended every render. This PR fixes that, and should also make the transitions API easier to implement for column, shape, and line series.

Note I didn't rebuild the project, just trying to keep the diff clean.

heavysixer commented 9 years ago

w00t this looks awesome! Let me review tonight when I get done with work. I assume all the tests are passing still?

nsonnad commented 9 years ago

Tests still pass, yes

heavysixer commented 9 years ago

One final question, I noticed this refactor effects several features but not all, are these just a sampling of a larger group that will need modifications or are you saying that these files are the only ones that benefit from the appendOnce change?

nsonnad commented 9 years ago

So these are the features where it was clear to me that there would be a benefit. The main reason for my not touching the others is that I am less clear on how they are implemented, so I didn't want to break them.

Essentially any feature that involves a data join should: 1) append once the wrapper element 2) use the resulting d3 selection to bind data and append g elements 3) selectAll for the visual elements and do another data join using a key function

Things like arrow, which are static, definitely don't need this. Looking through it looks like a few others would, though, like line-series-labels and stacked-column-connector. Let me know if the changes makes sense, and if so one of us can change the other features.

heavysixer commented 9 years ago

Perfect @nsonnad thanks for taking the time to answer my questions. I'll look through the commits tonight.

heavysixer commented 9 years ago

So I pulled these changes today and began to look them over. I think there is a slight error in the arc series, which is causing the donut chart to be drawn when the chart is updated. I've attached a screenshot to so you what I am seeing. I've not investigated it thoroughly yet and thought i'd check with you to see if you are seeing the same thing. screen shot 2015-02-26 at 12 08 47 pm

heavysixer commented 9 years ago

If you check out this repo: https://github.com/heavysixer/d4-www and start it running with "grunt server" and then start the d4 repo with "grunt watch" any changes you make to d4 will be copied over into the d4-www lib and you can try out the various examples to see the error i was getting. Make sure to touch a file in d4 however it won't kick off the initial build without it.

nsonnad commented 9 years ago

Hey @heavysixer, trying to get the examples running. Is there supposed to be a bower.json file in that repo? grunt server works but the browser is yelling at me about missing jQuery, d3, and bootstrap.

heavysixer commented 9 years ago

Hey @nsonnad I've added the bower.json file. Sorry about that!

nsonnad commented 9 years ago

Hello, I implemented this pattern for the remaining features that could use it. I have also added to d4-www a few charts that use dynamic data to test these implementations. See my dynamic-chart-examples branch here: https://github.com/nsonnad/d4-www/tree/dynamic-chart-examples

heavysixer commented 9 years ago

@nsonnad awesome! Let me check them out locally and then will merge them unless i have questions.

heavysixer commented 9 years ago

The code looks good, will you send me a PR for the dynamic charts on the d4-www project?