Skip to content
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

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from
Open

Paired row chart, again #1068

wants to merge 13 commits into from

Conversation

gordonwoodhull
Copy link
Contributor

This replaces #943, pulling just the relevant commits and updating for code standards.

Also includes rowChart.xAxisLabel since the example depends on that.

  • add tests for rowChart.xAxisLabel

@gordonwoodhull
Copy link
Contributor Author

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

@ruhley
Copy link
Contributor

ruhley commented Dec 17, 2015

@gordonwoodhull That looks great. Can't wait for some of this stuff to make it into the core library. 👍

@gordonwoodhull
Copy link
Contributor Author

So the formatting is a little bit messed up:

image

I also couldn't figure it out from looking at the demo on your fork, because it has different formatting problems:

image
http://ruhley.github.io/dc.js/examples/paired-row.html

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

@ruhley
Copy link
Contributor

ruhley commented Dec 17, 2015

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.

@gordonwoodhull
Copy link
Contributor Author

Huh, some kind of mixin, so that the code is shared. That's an interesting idea.

@gordonwoodhull
Copy link
Contributor Author

The design of #1053 is kinda hacky in different ways. 😀

@gordonwoodhull
Copy link
Contributor Author

Here's a branch with both versions included, for comparison and eventual combination into the best-of-both:
https://github.com/dc-js/dc.js/tree/pyramid-paired

I've run out of time for today!

@ruhley
Copy link
Contributor

ruhley commented Dec 17, 2015

I will look and try to get something done if I have some time.

@mtraynham
Copy link
Contributor

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?

@gordonwoodhull
Copy link
Contributor Author

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.

@gordonwoodhull
Copy link
Contributor Author

Here's the "mixed positive and negative stack" issue on d3. Guess it's an open problem: d3/d3#2265

@mtraynham
Copy link
Contributor

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

@gordonwoodhull
Copy link
Contributor Author

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.

@mtraynham
Copy link
Contributor

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

@gordonwoodhull
Copy link
Contributor Author

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.

@ruhley
Copy link
Contributor

ruhley commented Jan 1, 2016

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

@ruhley
Copy link
Contributor

ruhley commented Mar 7, 2016

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

@jennakwon06
Copy link

Do you have a demo for your implementation @ruhley ?

@gordonwoodhull
Copy link
Contributor Author

Hi @jennakwon06, I think I linked it above: http://ruhley.github.io/dc.js/examples/paired-row.html

@gordonwoodhull
Copy link
Contributor Author

Screenshot of the version in this PR and the pyramid-paired branch.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants