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 or Stacked bar chart with sensor bars #1470

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

Conversation

jaklub
Copy link

@jaklub jaklub commented Jul 17, 2018

Hey @gordonwoodhull !

Played around some more with grouped bar charts (after #1459) and tried a version inspired by @gazal-k PR #1043. Also included a version of the sensor-bars as discussed in #1428.

One "feature" of this PR is that you can easily switch between grouped and stacked bars.

One drawback is that this PR changes how the bars are "grouped/stacked" in the canvas, and their class names. Maybe something you would like to avoid?

@gordonwoodhull
Copy link
Contributor

I see. So we currently have the choice to be able to switch between grouped and composite easily, or switch between grouped and stacked easily.

I'd really like to support grouped + stacked at the same time, so I somewhat prefer the other PR (#1459). But it sounds like there are good arguments either way.

What deficiencies in the other approach caused you to move to this one?

@jaklub
Copy link
Author

jaklub commented Jul 31, 2018

Hey @gordonwoodhull !

Well, I started with this approach a litle bit as an exercise to get to know the library better.

One thing that "confused" me with the other approach (at least while working on the functions that calculated the bar widths and x coordinates :)), was how the bars are "grouped" in the dom.
It felt more intuitive that a "group" of bars would share a common parent element (see the screenshots bellow).

To accomplish that in this PR, I might have made some violence on how you are suppose to handle the stacked data coming from d3. As in these rows in plotData(): https://github.com/jaklub/dc.js/blob/da8f8748f5fc2b8cb2e6baf963c366aa74ec7561/src/bar-chart.js#L78-L115
Might be a better way to do that type of data pivot, or another way to achieve the same end result.

But after the pivoting I thought it was easier to work with the placement of the bars, and the injection of the sensor bars also felt natural. Also the change in behavior when hoovering over and selecting a group felt like it was easier to implement when the bars where grouped in the dom.

At first I set up the toggling between stacked and grouped bars in the examples to ease the tweaking of the width and x calculations. But after a while it felt kind of natural. I mean in a way the two ways of presenting the data could be considered to be really closly related. And treating them similarly in the implementation and the dom felt like an advantage.

But I agree one disadvantage of this approach is the support of grouped + stacked bars at the same time. That feature would probably be more difficult to implement.

Example of the dom in bar-grouped-stacked2:
bar-grouped-stacked2-dom

Example of the dom in bar-or-stacked:
bar-or-stacked-dom

@gordonwoodhull
Copy link
Contributor

Those are good points. I agree, to truly do it right, we want the DOM organized nicely and not just the API. I think that would be pretty difficult with the composite-based API, so I guess we would need something in the charts that understand more than 2D of data.

@cwolcott
Copy link

Has this feature stalled out. I have a need for the Grouped Bar Chart. I understand how long it can take to complete a new feature. I finally submitted the enhanced Box Plot a couple of months go, but actually started the enhancement 9 months prior.

@gordonwoodhull
Copy link
Contributor

Hi @cwolcott. I think the code is good. Unfortunately I stalled out from indecision between this approach and #1459.

The implementation here is much cleaner and IMO will be easier to maintain, and as argued above, it produces a cleaner DOM. However, this approach also pretty much shuts out the possibility of doing grouped+stacked bars for the foreseeable future.

Going either way is going to make people unhappy. I should probably pull the trigger and decide one way or the other, but it's going to cause me some pain too, either trying to maintain the other one or hurting my head trying to make grouped+stacked work with this one.

@cwolcott
Copy link

Understand. I am actually looking for group+stacked.

@gordonwoodhull
Copy link
Contributor

There's a branch mentioned in the other ticket, which has a reasonably up-to-date version (3.0.5) of dc.js with grouped+stacked. If you're okay with the limitations of using a branch, it could be a solution for you.

@Frozenlock
Copy link
Contributor

I'll just chip in and describe how I'm currently using the other branch.
I split a timeseries into multiple different dimensions (say weekly) and compose them all into the same chart.

For example, each color represents a different week :
image

Is this something that could be done with this PR?

@gordonwoodhull
Copy link
Contributor

Hi @Frozenlock, I think that scenario can work with either grouped bar implementation, at least the drawing part.

The stack feature just takes any group (no matter what dimension it belongs to), and grouped bars in this PR are implemented using the stack interface, so that should work.

The filtering behavior might be different, though. Currently there is no support for selecting individual segments of a stacked bar chart (#657), and that applies to the other implementation of grouped bars too. (It's really a data problem, since there is usually no dimension representing the stacked-ness). With this implementation, each bar in a group is in its own chart, so it is possible to filter it.

(I haven't verified this, just going on my general understanding of how stuff works.)

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