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 bar chart #1043

Closed
wants to merge 15 commits into from
Closed

Grouped bar chart #1043

wants to merge 15 commits into from

Conversation

gazal-k
Copy link
Contributor

@gazal-k gazal-k commented Nov 20, 2015

Modification of #679

  • Hopefully I have resolved most of the conflicts with dc.js mainline.
  • Resolved a few issue in rendering multi/grouped bar chart.
  • I've added some basic tests for multi/grouped bar chart.

Pending:

  • not sure what functionalities expected of bar charts in general is missing in the multi/grouped version.
  • multi/grouped row chart.

Please feel free to suggest changes or fork this and fix further.

wssbck and others added 7 commits August 20, 2014 21:47
Conflicts:
	src/bar-chart.js
	src/composite-chart.js
	src/coordinate-grid-mixin.js
	src/legend.js
	src/line-chart.js
	src/margin-mixin.js
	src/pie-chart.js
	src/row-chart.js
	src/series-chart.js
_chart.groups().forEach(function (g, k) {
var item = {
chart: _chart,
name: g.title,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this approach doesn't seem consistent. the legend name for the components will appear only if each group has a property 'title'

@gordonwoodhull
Copy link
Contributor

Oh wow, now we have two competing versions of grouped bars. See #984. This is going to get interesting.

The issue I have with this one, if I remember correctly, is that @wssbck made a ton of unrelated changes in this branch. Enough so that at the time, I couldn't make heads or tails of it.

However, this is less of an issue now that you've fixed the tests, @gazal-k. Thank you! Since you've reviewed it, can you summarize what the other changes are?

@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 20, 2015

@gordonwoodhull I didn't really know about #984. I originally had a hack which involved some transform = translate() style post render tricks to get multi/grouped bar charts functionality within composite chart. Then I saw the discussion between you and @wssbck about how its probably not the best idea to do grouped bar within composite.

@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 20, 2015

I also had a hard time figuring out all of the code here. I don't have a comprehensive idea of how the framework gets things done. I just meddled with it.

This is what I understand about it:

  • There's a multigroup-mixin which abstracts some functionality for grouped bar charts and row charts.
  • The grouped bar chart works for the most part after some tweaking; I was able to write some basic tests.
  • Yet to check the grouped row chart

If you can please check some inline notes I have added and help me, that'd be really awesome.

avoiding the new groups property
chart data function from stack mixin tweaked to get grouping function
no separate legendables function required in group mixin
@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 21, 2015

@gordonwoodhull I did take some inspiration from #984 to make the code cleaner.

With the current approach I was able to fix title rendering, and labeling groups became more consistent.
I liked how @alan-unravel was able to get the functionality within dc.barChart.

@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 21, 2015

@gordonwoodhull I think I finally figured out the heads and tails of it 😄

Got the grouped bar chart functionality to work within bar chart code.

@gazal-k gazal-k changed the title [WIP] Multi/grouped bar and row chart Grouped bar chart Nov 21, 2015
layer.values.forEach(function (value) {
value.y0 = 0;
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure whether this is the best approach to get this done

@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 22, 2015

Fixes #558

@gazal-k
Copy link
Contributor Author

gazal-k commented Nov 22, 2015

got to get centerBar to work properly when groupBars is set.

Another thing I noticed, with ordinal x axis, the bars seem to already be centered (with single group, stacks both vertical and horizontal) and applying centerBar(true) seems to break it. How should this be ideally?

ignoring centerBar for ordinal charts
x += layerIndex * (_barWidth + _gap);
x += _gap / 2;
}
if (_centerBar && !_chart.isOrdinal()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this check; _chart.isOrdinal() affects bar charts in general.

I saw that bar charts with ordinal x axis are already centered around axis points. So setting centerBar(true) only seems to offset it incorrectly.

Please see if this is fine

@madslundgaard
Copy link

Dear @gazal-k and @gordonwoodhull,
How far would You think this is from being submitted to development branch. I am working on a project where grouped bar chart is a critical chart type (for version/scenario comparison).

I would like to help on the issue if you believe that additional input/testing is needed (basic JS,D3,DC experience).

@gazal-k
Copy link
Contributor Author

gazal-k commented Jan 6, 2016

@madslundgaard you needn't wait for this PR to get merged if u want to use this immediately. U can create a fork of dcjs and merge my branch to yours and use that instead of referring to dcjs base fork.

If you are using bower, the dependency can be something like: madslundgaard/dc.js#develop

@gordonwoodhull
Copy link
Contributor

Yes, @madslundgaard, in fact testing the feature and providing feedback, and code review if you're up for it, is the best way to help it get into dc.js. Adding more tests is always welcome too!

Note that @mtraynham and others are discussing adding another mixin to provide a uniform interface for "extra dimensions" like this one. I don't think it will change the external interface too much, just help with the code organization.

@slackhideo
Copy link

This versions of grouped bars works well.
Thanks, @gazal-k and @gordonwoodhull for your effort.

I noticed that the ordering and barPadding functions do not work together with groupBars. Is there a way I can have grouped bars working with these two?

Thanks.

@gordonwoodhull
Copy link
Contributor

Hi @slackhideo, thanks for your review. Have you also compared #984?

I'll let @gazal-k speak to whether he has time to add ordering and barPadding support.

@slackhideo
Copy link

Hi, @gordonwoodhull. Thanks for the reply.

Actually I was wrong about the ordering support. It does support this function (sorry!).
Instead of using chart.ordering(function(d) { return -d.value; }) I needed to use chart.ordering(function(d) { return -d.value['1']; }) (to order according to the layer '1').

But I'm afraid I have no idea how to tackle the barPadding support.

@gordonwoodhull
Copy link
Contributor

@slackhideo, have you also tried #984?

@slackhideo
Copy link

Oh, sorry @gordonwoodhull, I forgot that part.
Now I tested #984 and found that it supports barPadding.
Thanks for the suggestion.

I've chosen this implemention because, judging by your example, this version looks better to me. The groups are more aligned with the X-axis' ticks (turning off/on centerBar didn't help here) and the spacing between bars in the same group seems more pleasant. I don't know if these things are easily changeable.
I guess both versions have their strengths and shortcomings.

@gazal-k
Copy link
Contributor Author

gazal-k commented Jun 29, 2016

@gordonwoodhull @slackhideo sorry, its been quite a while since I've gotten my hands dirty with dc.js. I've been and still am tied up with some other work. I'd love to contribute when I get some free time. But until then, sorry 😞

@gordonwoodhull
Copy link
Contributor

Thanks @slackhideo for your review. That's very helpful.

And thanks @gazal-k for all your contributions! Great stuff.

I think the issue might be that we need two levels of barPadding for this chart. But my memory is a bit fuzzy. I'll try to take a look when I can.

@slackhideo
Copy link

Thank you, @gordonwoodhull and @gazal-k. Your work is amazing, really.
It's just some minor things that can be improved (well, almost everything can be improved), but overall is a stunning library and this grouped bar feature is very useful.
Your hard work is appreciated!

@frankisans
Copy link

Hi @gordonwoodhull I have check both branches on a composite char with a line chart and a grouped bar chart. I prefer gazak branch because no gap on same group bars:

gazak:
image

kavanagh:
image

I use another control to change the focus dinamically but the bars overlap the yAxis:
image

Seems not a problem with grouped bar chart implementation because gets the same result with normal stacked bar. I cannot use padding because elasticX is false for focus to work.

@gordonwoodhull gordonwoodhull modified the milestone: v2.1 Dec 2, 2016
@suntong
Copy link

suntong commented Jan 12, 2017

  • All checks have passed
  • This branch has no conflicts with the base branch

Waiting for the merge...

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jan 12, 2017

Thanks @suntong, I agree this is very close to being ready to merge (and it will be the first thing to go into 2.1). However, there are a still a few questions raised by the author which I need to address, as well as manual code review and testing.

Please go ahead and use the version in this branch:
https://github.com/dc-js/dc.js/tree/gazal-grouped-bar

I've pulled the latest changes from develop, so this has all of the fixes from dc.js 2.0.

I apologize for the long turn-around but unfortunately this is not my full-time job (however much I wish it were).

EDIT: direct rawgit link to the library:
https://rawgit.com/dc-js/dc.js/gazal-grouped-bar/dc.js

@suntong
Copy link

suntong commented Jan 12, 2017

Definitely, totally understand. I was just trying to say something meaningful in order to subscribe to this thread. Take your time.

@parliament718
Copy link

@gordonwoodhull Could you by any chance merge develop again? 57 commits and almost 3 months behind and Im having issues integrating @types/dc, which follows this develop branch.

@fatangare
Copy link
Contributor

Does it work with time scale or only with ordinal?

@Frozenlock
Copy link
Contributor

Very interested in using grouped bar chart!

Is there still a strong possibility of it being added soon in the official library? In other words, is it wise to start building on top of this?

@gordonwoodhull
Copy link
Contributor

@fatangare, because it build on the stacked chart, it will work wherever that does. So yes.

@Frozenlock, yes, I hope so. This is one of a few major PRs ready to merge before the big port to d3v4.

@OivalfMarques
Copy link

This solution does not allow having bar chart multiple series and staked at the same time, but even so this solution moves away from the rest of the library. What I propose is that instead of looking at the number of stacks, start looking at the number of bar chart, if this is a composite chart.

Follow the implementation link on the latest version of this library

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

I'm waiting for an feedback

Thank you for your time.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 11, 2017

Hi @OivalfMarques, do you mean that you have an implementation of stacked and grouped charts? That would of course be fantastic.

What do you mean by "implementation link" - this looks like it's a demo of composite + stacked charts but nothing grouped?

@Frozenlock
Copy link
Contributor

@gordonwoodhull This indeed looks like an implementation of grouped and stacked charts, which is amazing. (You can see that the gap is a little wider between groups.)

@OivalfMarques Amazing work! Do you have a fork available with your changes? PR?

@OivalfMarques
Copy link

OivalfMarques commented Dec 12, 2017

hi @Frozenlock yes i have. So you will do merge?

@gazal-k
Copy link
Contributor Author

gazal-k commented Feb 14, 2023

This is most likely unnecessary at this point.

@gazal-k gazal-k closed this Feb 14, 2023
@gazal-k gazal-k deleted the multi-bar-row-chart branch February 14, 2023 00:14
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.