-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Grouped bar chart #1043
Conversation
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
src/multigroup-mixin.js
Outdated
_chart.groups().forEach(function (g, k) { | ||
var item = { | ||
chart: _chart, | ||
name: g.title, |
There was a problem hiding this comment.
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'
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? |
@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. |
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:
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
@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. |
@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. |
layer.values.forEach(function (value) { | ||
value.y0 = 0; | ||
}); | ||
}); |
There was a problem hiding this comment.
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
Fixes #558 |
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()) { |
There was a problem hiding this comment.
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
Dear @gazal-k and @gordonwoodhull, I would like to help on the issue if you believe that additional input/testing is needed (basic JS,D3,DC experience). |
@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: |
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. |
This versions of grouped bars works well. 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. |
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. |
Hi, @gordonwoodhull. Thanks for the reply. Actually I was wrong about the ordering support. It does support this function (sorry!). But I'm afraid I have no idea how to tackle the barPadding support. |
@slackhideo, have you also tried #984? |
Oh, sorry @gordonwoodhull, I forgot that part. 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. |
@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 😞 |
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. |
Thank you, @gordonwoodhull and @gazal-k. Your work is amazing, really. |
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: I use another control to change the focus dinamically but the bars overlap the yAxis: 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. |
Waiting for the merge... |
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: 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: |
Definitely, totally understand. I was just trying to say something meaningful in order to subscribe to this thread. Take your time. |
@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. |
Does it work with time scale or only with ordinal? |
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? |
@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. |
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. |
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? |
@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? |
hi @Frozenlock yes i have. So you will do merge? |
This is most likely unnecessary at this point. |
Modification of #679
Pending:
Please feel free to suggest changes or fork this and fix further.