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

Grouped and Stacked Bars2 #1459

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

Conversation

jaklub
Copy link

@jaklub jaklub commented Jul 3, 2018

Updated @OivalfMarques grouped-stack-bar-chart #1356 to work with version 3 of dc.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jul 3, 2018

Thanks @jaklub! I have some ideas about a cleaner, easier-to-maintain implementation, but you are right that porting this to v3 is the first step.

For now, could you please add an example of grouped+stacked bars? The grouped-bar example is very helpful but it would be great to see the whole thing in action.

@gordonwoodhull
Copy link
Contributor

Also, if you're up for it, could you also enable brushing and put another chart into the example? Let's start a discussion about how interaction should work.

I tried enabling brushes in the original plunkr in the previous PR, and it looks like brushing does not work on the grouped&stacked chart:

  • bars are not greyed out when they are outside the brush (until it is redrawn, e.g. when the other chart is filtered)
  • filtering doesn't happen when brushing the grouped&stacked chart

https://plnkr.co/edit/3q8pK4NGHYw6h7Rck7t0?p=preview

It looks like you may have done some additional work in this area?

I think this can be made to work, although it would have to round the brush so that a whole group is selected at once, not just individual bars.

@jaklub
Copy link
Author

jaklub commented Jul 3, 2018

Hey @gordonwoodhull! I was using one of the earlier grouped barchart PRs, but I kind of wanted to use the new shiny dc v3 :). So I tried to cook up something that worked (at least for me).

But I must admit, I mostly considered my own use cases when creating this PR. Didn't for instance try to make it work with both grouped and stacked bars in the same chart as in the plunkr of the previous PR. Is that something that you would like to be working?

Tried quickly with brushing also, but don't think it worked. I can look into it bit more.

Added one more example though, with two grouped and filtering barcharts.

And yes, I did work some on the select/deselect portion of the code. It kind of works for my use cases, but it dosen't feel clean. I did add a few lines of code that overrides the click event for instance, feels a bit like a hack.

I am a bit of a beginner with this library (and d3), so please point out my newbie mistakes :). And by the way thanks for all work with dc, its a great lib.

@Frozenlock
Copy link
Contributor

Frozenlock commented Jul 4, 2018

Unfortunately it doesn't work.

Previous version :
image

This branch :
image

In addition, the new branch causes my CPU to run wild.
The CPU problem was caused by my custom composite-filter-handler.

@jaklub
Copy link
Author

jaklub commented Jul 4, 2018

@Frozenlock I've made a jsfiddle https://jsfiddle.net/jaklub/8dcsmbuk/ could you try to fork and modify to reproduce your problem?

@Frozenlock
Copy link
Contributor

Hmm... I get an error about .bandwidth when I try to switch the jsfiddle to a non-ordinal chart.

  .x(d3.scaleLinear().domain([0, 9]))
  .xUnits(function () {return 10;})

Did I forget something?

https://jsfiddle.net/hnaumer2/

@jaklub
Copy link
Author

jaklub commented Jul 5, 2018

@Frozenlock I've updated the grouped barchart to be able to handle non ordinal charts. Also found small mistake in the fiddle when using the non ordinal bar chart.

dim = ndx.dimension(function(d) {return "Run " + d.Run;}),

=>

dim = ndx.dimension(function(d) {return d.Run;}),

https://jsfiddle.net/jaklub/hnaumer2/5/

@gordonwoodhull: Played around with the brush, it seems to be working somewhat now. Added a new example with three charts, one with the brush on.

@gordonwoodhull
Copy link
Contributor

Hi @jaklub - awesome, thanks for following through and providing these fixes. The examples looks very good.

It strikes me that we might want to change the ordinal-grouped hover and click behavior. Since you always get the whole group rather than an individual bar, it seems to me that

  1. the hover highlight should affect all of the bars of the group, not just the individual one the mouse is over
  2. possibly we should allow clicking in the area behind the bars, i.e. the click target is the group itself, not the bars.

These are just ideas for future work.

My main concern with merging this remains the way the chart reaches into its parent the composite chart and its siblings, in order to decide how to draw. I feel that this could be clearer and easier to maintain, but I don't yet have a concrete plan.

Until then, thanks for bringing this PR into the new world and making it available for everyone to try out!

@Frozenlock
Copy link
Contributor

@jaklub Now it works as expected.

image

Thank you!

@jaklub
Copy link
Author

jaklub commented Jul 6, 2018

@gordonwoodhull - I Agree that would be nice, to have the groups behave more like a group when hovering and selecting.

In my opinion, this implementation makes the "physical" grouping of the bars a bit warped from how you might perceive them as a developer or user. I mean each "bar group" contains one bar from each child in the composite. This approach might also make it more difficult to implement something like you are suggesting above (and implementing #1428 which also sounds like a great idea). And I guess this also forces the charts to reach out to its siblings.

I was looking through some of the earlier grouped bar chart versions. Think I was using the @gazal-k branch from #1043 earlier myself. Do you think that would be a better approach? To use stacking and an extra property like groupBars? Would of course make it more challenging to implement grouped and stacked bars in the same chart (don't know how requested that feature is though).

Or do you have any other thoughts on how to implement this feature in an more maintainable way?

Would love to see grouped bar charts, in one form or another, eventually make its way to the master branch :).

@gordonwoodhull
Copy link
Contributor

One of the arguments from the previous PR that made me prefer this approach, is that you can switch between a composite of line charts and stacked bars with mostly the same code.

What's missing, I think, is a class that sits between the composite chart and the bar chart and says "you are part of a group". This class would figure out the offsets of the bars within their groups, and could also mediate the mouse events we're talking about here. Then the offset is just another property of the bar chart and hopefully it doesn't have to look at its siblings.

I'm glad you mentioned #1428 - that's exactly what I was thinking for "clicking on the group".

@grugknuckle
Copy link

@gordonwoodhull I've been using the dependency

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

in my project. It was the dependency you had suggested in issue #1356 . But recently when I went to build my project I got an NPM warning

Downloading the git repo "git://github.com/dc-js/dc.js.git" over plain git without a commit hash

That tells me that NPM was looking for a specific commit, but that perhaps the hash tag #grouped-stacked is no longer valid(?). If that is in fact the case, what is the correct dependency to use? If possible, I would like to know both the dependency for the grouped-stacked PR based on dc v2.1.x and the grouped-stacked PR based on dc v3.0.x.

@gordonwoodhull
Copy link
Contributor

Hi @grugknuckle, I wasn't able to repro that with [email protected], node@10. When I use the above line in package.json it installs a version of dc.js with the header

/*!
 *  dc 2.1.10-groupstack
 *  http://dc-js.github.io/dc.js/
 *  Copyright 2012-2016 Nick Zhu & the dc.js Developers
 *  https://github.com/dc-js/dc.js/blob/master/AUTHORS

which is what I expect.

@grugknuckle
Copy link

Ok ... Thank you @gordonwoodhull . Apologies for polluting this PR with my troubleshooting problems.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jul 10, 2018

No problem, @grugknuckle, it's good to be aware of these issues. I would just check that it's installing what you expect and not worry about it unless it really is dropping the branch info.

For dc.js 3.0 compatibility, I also pushed a branch for this PR; you should be able to use:

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

This will install node_modules/dc/dc.js with the following header:

/*!
 *  dc 3.0.5-groupstack
 *  http://dc-js.github.io/dc.js/
 *  Copyright 2012-2016 Nick Zhu & the dc.js Developers
 *  https://github.com/dc-js/dc.js/blob/master/AUTHORS

@grugknuckle
Copy link

grugknuckle commented Jul 10, 2018

Excellent ! Thanks @gordonwoodhull .

@Frozenlock
Copy link
Contributor

Would it be possible to update the grouped branch with the newest release?

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Oct 3, 2018

Okay, the branch grouped-stacked-3 now has version

3.0.7-groupstack

3.0.8-groupstack

Sigh, really hope to resolve this deadlock soon. I am not completely happy with either solution but these sure are useful features!!!

@Frozenlock
Copy link
Contributor

Thank you very much!

@dadokkio
Copy link
Contributor

Hi all, I'm playing with 3.0.8-groupstack and it works very well. I've just a question.

I have a dashboard with a lot of other charts and one of this is a piechart with the values that the groupchart is using for his x axis.

Example
The two pie filters each other but only the second one (that is not using exp as dimension) works also for the groupedchart and viceversa.

It's that correct?

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Mar 11, 2019

Hi @dadokkio, that has nothing to do with this chart. It's because a group does not observe its own dimension's filters. If you want two charts to filter each other, they need to be on different dimensions.

@Frozenlock
Copy link
Contributor

Hello @gordonwoodhull,

Have you made a decision as to which implementation should be merged into master?
(#1459 or #1470)

@gordonwoodhull
Copy link
Contributor

I dislike both implementations for different reasons. I am currently of the opinion that stacking needs to be expanded to multiple dimensions, but I haven't tried it.

Sorry for the disappointment. Maybe someone should fork and maintain their own version of this.

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.

6 participants