Grouped and Stacked Bars

[OivalfMarques]

Allow grouped and stacked bars, based in Grouped bar chart #1043

https://plnkr.co/edit/V99X09LPMNc0V7qP3jum?p=preview

[gordonwoodhull]

Thanks @OivalfMarques! Now I see how this works - sorry I missed it the first time.

I'm a little bit uneasy about reusing the composite chart for this purpose, but it certainly is an unambiguous and convenient way to get this done.

I am going to have to think about this approach further. @Frozenlock and others, if you have an interest in the combination of stacked and grouped charts, could you try out this PR and leave reviews?

[Frozenlock]

@gordonwoodhull Will do.

What's command to compile the js files?
I've generated the dc.js file using "grunt test", but I think it's still the old version; I get an error associated with getCharts (one of the new functions).

[Frozenlock]

After double checking, I'm pretty sure I'm using the correct version...
Yet I get this :

Uncaught TypeError: parent.children is not a function
    at getCharts (dc.js:5630)
    at calculateBarWidth (dc.js:5757)
    at Object._chart.plotData (dc.js:5602)
    at drawChart (dc.js:3922)

[Frozenlock]

@OivalfMarques Could it be that the new bar chart can't accept a node as a container?

[gordonwoodhull]

Sounds like the code should use dc.instanceOfChart in getCharts, the way we do in baseMixin.anchor:

https://github.com/dc-js/dc.js/blob/develop/src/base-mixin.js#L433

(There are a few valid types for parent; it's not just string or composite chart.)

[OivalfMarques]

@Frozenlock, @gordonwoodhull Hi thanks you for your suggestion.
I've already corrected.

[gordonwoodhull]

Did that fix it for you, @Frozenlock?

[Frozenlock]

Yup, it's working great!

I've encountered some issues with highlighting the legendables, but I'm not sure if it's my setup.
I should have a better idea by Sunday.

[Frozenlock]

I've looked further into the legendables issue.

Here's my understanding of how it works :

• Legendables for a composite chart consists of returning the legendables of each children.
• In this case, each children doesn't return itself as a legendable, but rather goes deeper and return every stacked bar.

    _chart.legendables = function () {
        return _stack.map(function (layer, i) {
            return {
                chart: _chart,
                name: layer.name,
                hidden: layer.hidden || false,
                color: _chart.getColor.call(layer, layer.values, i)
            };
        });
    };

So... how is one supposed to highlight all the stacks from a single bar chart?

[gordonwoodhull]

This PR is very clever, but as I understand it, this is not really a composite chart but it is using the composite chart as a handy place to store extra data.

That said, there is probably some way to hack the sort of legend highlighting that you want, @Frozenlock. I take it you would like to highlight vertically an entire bar, including all of its stacks?

(I can also imagine wanting to highlight a particular stack, across all of the bars no matter which "group" they are part of.)

One thing that's kind of unfortunate in dc.js is that the legend highlighting actually selects the bars by color (!!) So you might have to go in there and override the whole legendHighlight and probably legendables too (defined in the stack mixin), if you don't want to dig in and start fixing the library itself.

[Frozenlock]

@gordonwoodhull Thanks for the highlighting info.

This PR is very clever, but as I understand it, this is not really a composite chart but it is using the composite chart as a handy place to store extra data.

While it might be wise to eventually store the extra data elsewhere, I think that using the composite approach is the simplest yet for the user.

After all, from a user point of view (me), it's a composite chart :
I have 2 or more charts that I want to merge together.

Also, it allows us to use the same code for handling both line and bar charts, which is a big advantage.

[grugknuckle]

Has there been any update on this? Grouped bar charts is something dc.js desperately needs.

[Frozenlock]

I've been using this PR for months. Hopefully it will make it into the next release.

[gordonwoodhull]

Hi @gregknuckle, I'd advise you to go ahead and use either of these PRs if you need the functionality now.

I am still deeply conflicted by the approach taken in this PR. This twists the meaning of what a composite chart is, and would be hard to maintain because it makes the code quite convoluted.

So I don't see myself merging this as it is.

But you can still use it! And when dc.js gets this feature, it will be with at least as easy an interface, so it will not be hard to port your code over.

Thanks!

[grugknuckle]

That's all great and stuff, but I don't know how to npm install a specific pull request, and even if I did, I don't think it would be a good idea. Because when you do make the next release, I'm certain to 'npm update dc' and then where would I be? Either in a code review explaining to my supervisor why we can't have the latest and greatest, or locked away in my cube frantically trying to hack this pull into the new release. So I'll pass on the PR.

That said, my enhancement request stands. dc.js is a great library, but it needs a grouped bar chart. I didn't look at the code in this PR to see why it would be hard to maintain. So I will take your word for it @gordonwoodhull. But I think @Frozenlock is correct when he says that the 'natural' way to think of grouped bar charts is as a composite chart of two or more bar charts. I mean, you can put two line charts in a composite. and you can put a line chart and a bar chart together. What would a composite chart of several bar charts look like? It would have to be stacked bars or a grouped bars.

Anyway ... that's my 2 cents. Thanks for the quick responses guys.

[gordonwoodhull]

Hi @Frozenlock, thanks for your comments.

I definitely appreciate what you are saying about a clean reusable interface, and also how you can do multiple-series line charts in the same way as grouped bars. (I think multiple-series stacked line charts would be pretty confusing! So that part is not the same, but the rest is.)

So yes, you've convinced me that the composite of bars makes sense for groups.

My remaining concern is with the implementation, where each bar chart reaches out and looks at all of the sibling bar charts in the composite in order to draw itself.

[gordonwoodhull]

In the meantime, I've pushed a branch grouped-stacked which is this PR rebased onto develop.

@grugknuckle, you can change your dependency to

    "dc": "git://github.com/dc-js/dc.js#grouped-stacked",

and use this branch.

Hope to figure out a way to merge this soon. Thanks for the insightful discussion.

[grugknuckle]

Hey @gordonwoodhull, I've been playing with this branch

"dc": "git://github.com/dc-js/dc.js#grouped-stacked"

and found some bugs having to do with the titles. Should I report them in the issue tracker of the main branch? Or do you want me to put them somewhere else?

[gordonwoodhull]

Thanks @gregknuckle! Could you report issues right here on the PR?

We'll make sure any issues are resolved before merging.

[grugknuckle]

@gordonwoodhull @Frozenlock

So ... I was just in the process of making a really long post to describe the bug I was seeing. And while I was trying to explain what I was seeing, I realized it was just this bug ...

#554

So, sorry for the false alarm. But also - HA! - thanks for helping me fix it!

[gordonwoodhull]

Thanks @grugknuckle - I take it .shareTitle(false) fixed it for you?

[grugknuckle]

No. What fixed it was to put .shareTitle (false) BEFORE .compose on the composite chart.

EDIT: I'm on my phone so I won't elaborate. But I found that solution somewhere in issue 554.

[gordonwoodhull]

Got it, thanks!

That is definitely weird behavior. In general, the configuration code should not have any side effects; anything that happens should happen when the chart is rendered / redrawn. We violate that all over the place, though.

[Frozenlock]

Would it be possible to push the recent changes into the grouped-stacked branch?
I'd like to try the shiny new D3 😃

[gordonwoodhull]

Thanks to @jaklub for providing a new version of this PR for dc v3! See #1459.

I still think the implementation needs to change to make the code easier to understand and maintain. However, I agree this is a good interface for grouped & stacked bars.

I'm going to close this PR and move discussion to the new PR. Will try to merge to v3.