-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Paired row chart, again #1068
base: develop
Are you sure you want to change the base?
Paired row chart, again #1068
Conversation
@ruhley, I don't think I got quite all of the formatting and code from your branch, as I just took the following commits so far: |
@gordonwoodhull That looks great. Can't wait for some of this stuff to make it into the core library. 👍 |
So the formatting is a little bit messed up: I also couldn't figure it out from looking at the demo on your fork, because it has different formatting problems:
(note that the row chart is completely faded out, too) So it's kind of hard to do a fair comparison between #1053 and this one. I like the design and reuse of code better on this version, but maybe it's easier to get it right if it's just another chart with all the code duplicated. Anyway, I'm creating a special branch just to compare and merge the two, and will continue working on it. |
Yea, I always felt it was a little hacky just making two sub row charts. Duplicating the code or creating a row chart mixin would definitely be a more robust solution. |
Huh, some kind of mixin, so that the code is shared. That's an interesting idea. |
The design of #1053 is kinda hacky in different ways. 😀 |
Here's a branch with both versions included, for comparison and eventual combination into the best-of-both: I've run out of time for today! |
I will look and try to get something done if I have some time. |
Does this only support pairing? Would there ever need to be a case that we support "stacking" or rather more than 2 domain values for each row? |
Sure, people want stacked row charts all the time. @RowanHart reported some progress on his fork in #397 but it never made it into a PR, due to the difficulty of writing tests IIUC. :( I don't think that really helps us here since d3.stack doesn't do bidirectional as far as I can tell. (Maybe this is an oversight since it does support streamgraphs?) So there is still the matter of somehow tracking two row charts and combining them, and I somewhat prefer the approach here of combining the charts, over the approach in #1033 of creating a whole new chart, although that one is a little more stable and there are good arguments for each. As @ruhley says, some kind of row mixin might be the best of both worlds. I know it's not helpful to bring it up, but just to stir the pot 😈, bar charts might also want bidirectionality, i.e. negative stacks. And both row charts and bar charts already support negative bars. |
Here's the "mixed positive and negative stack" issue on d3. Guess it's an open problem: d3/d3#2265 |
I have a stacked version that works pretty well 😄 ... I was a bit hesitant to add it, since it's a bit different from the dc.js implementation and kind of a one off (uses my own stack mixin which allows missing data points, and supports scrolling for fixed row heights). I can post the code if you're interested (the less patched code I have to maintain the better). |
dc.js is diverging... I can't keep up with all the pull requests and a lot of people (like @ruhley) have live forks which would be pretty difficult to merge. I think this is normal and healthy, actually, but it can be rather confusing, especially for me. I will merge a lot of these but it's also great to have so many "wild" forks. So yes, @mtraynham, please at least push to your fork, or if you can handle the overhead, make a feature branch and submit a PR. Making sense of this area of stacks and groups and multiples is of particular interest to me and it's helpful to have many implementations to compare and combine. |
Well in the best interest of everyone else using the library, we should set some shared goals on the design of these charts. I've seen quite a few variations between the PR's already and even my own version seems somewhat silo-ed in purpose. At the same time, I would prefer we keep complexity of supporting 3rd and 4th dimensional charts to a minimum. The less maintenance required going forward would really help make additional features easy to implement. My post from a while back on the Google Group I thought was a good starting point, but it never really took off in terms of feedback from others. https://groups.google.com/forum/#!topic/dc-js-user-group/Rj_u-D5ygEk |
Wow, really great post and I have arrived at the same conclusion after reviewing all these submissions. I see that I didn't read it closely enough because I was concerned about holding back contributors while working out a perfect design - but what's happened is that a lack of a good design has held me back from merging these PRs! I'll resurrect that thread soon, once I've read your code and thought about it some more. My gut feeling is that both data formats should be supported, depending on the desired filtering behavior. I.e. if one wants to allow filtering on sub-dimensions then one must use multidimensional keys, but if not then multidimensional values should be preferred because they are more efficient (less bins for crossfilter to handle). Either way the data will be transformed by a mixin so that the individual chart doesn't need to be concerned with the original format. Bravo. |
I have started to implement a row mixin for the row chart and paired row chart. I think it is the best solution. I will see if the stack mixin can easily be added on top of the row mixin. @mtraynham The layer mixin looks like it could be a really great addition for dc.js |
@gordonwoodhull I have been very busy lately and my version of dc.js fell behind a bit. Today I managed to rebase my develop branch up to yours. I then reimplemented everything that I had before (except for the responsive stuff as I see others have different solutions and the proper choice needs to be made). I have an initial version of the row chart and paired row chart based on a row mixin. As you can see the https://github.com/ruhley/dc.js/blob/develop/src/row-chart.js and https://github.com/ruhley/dc.js/blob/develop/src/paired-row-chart.js files are fairly simple with most of the logic in https://github.com/ruhley/dc.js/blob/develop/src/row-mixin.js I feel like it is a much better solution with no workarounds or hacks. There may still be some bugs and options that I haven't considered yet, but the standard display and crossfiltering works great. |
Do you have a demo for your implementation @ruhley ? |
Hi @jennakwon06, I think I linked it above: http://ruhley.github.io/dc.js/examples/paired-row.html |
This replaces #943, pulling just the relevant commits and updating for code standards.
Also includes
rowChart.xAxisLabel
since the example depends on that.rowChart.xAxisLabel