Group stacked bar charts (#2643) #3563

potatopeelings · 2016-11-07T13:28:39Z

Code changes, documentation update and sample for #2643.

JSFiddle: https://jsfiddle.net/Lvj2qnrp/1/
(same as samples/bar/bar-stacked-group.html)

etimberg · 2016-11-07T23:55:39Z

docs/04-Bar-Chart.md

@@ -49,6 +49,7 @@ borderSkipped | `String or Array<String>` | Which edge to skip drawing the borde
 hoverBackgroundColor | `Color or Array<Color>` | Bar background color when hovered
 hoverBorderColor | `Color or Array<Color>` | Bar border color when hovered
 hoverBorderWidth | `Number or Array<Number>` | Border width of bar when hovered
+stack | `String` | For grouping stacked bars


Is String the best type for this? Would a number make more sense?

I think String is the best, because it serves as the name of the stack. For instance, here's chadcodes' image from the related issue

Also a number would imply an order (confusing if Dataset1 and 2 are in stack 2 and Dataset 3 is in stack 1)

On a slightly related note - do you think stack is a generic enough property name, or would something like group (or cluster) be better?

I think that's a good call then. You're right that the numbers could imply an order to the stacks.

I think stack is ok. Maybe @simonbrunel has some thoughts on this

I was thinking group first, but finally stack might be better. Can we explicitly document this dataset.stack as an ID and not a name to avoid any confusion with label/text/title?

I was thinking group first, but finally stack might be better.

After I chose stack, I was worried that it would be very bar specific (just in case we want to use the same property for grouping datasets for other chart types) + we are calling it grouped stacked bars. But I'll go with whatever you suggest (I've being going back and forth between them that they both seem logical - I'll have good things to say about stack once we've changed to group :-))

Can we explicitly document this dataset.stack as an ID and not a name to avoid any confusion with label/text/title?

You mean call it stackID (like we do with axisIDs) and point to options (in future) for name, styling? Or do you mean just change For grouping stacked bars to ID for grouping stacked bars? Should be simple enough either way.

I mean simply change the documentation: identifier to group bars in stack(s) (or similar) but keep the property name simple: stack. About the stack vs group, group would make sense if this concept could be implemented for every types of chart, which I'm not sure it's possible, right?

Ok done!

About the stack vs group , group would make sense if this concept could be implemented for every types of chart, which I'm not sure it's possible, right?

I was thinking about folks who extend Chart.js to add other chart types (outside of the core library). If we call this stack, they may be tempted to add / use a new property that's better suited to their chart type.

A convoluted example would be a grouped scatter plot (with pie charts instead of dots). stack feels out of place, group might be ok.

etimberg · 2016-11-07T23:56:07Z

src/controllers/controller.bar.js

 		},

-		// Get the number of datasets that display bars. We use this to correctly calculate the bar width
+		// Correctly calculate the bar width accounting for stacks and the fact that not all bars are visible
 		getBarCount: function() {


Might be a good idea to have a unit test that tests this new functionality by creating a chart with multiple stacks

Agree. I haven't worked with karma yet. Will read up a bit and add tests.

Added unit test. Cheers!

etimberg · 2016-11-08T00:01:51Z

src/controllers/controller.bar.js

 			if (xScale.options.barThickness) {
 				return xScale.options.barThickness;
 			}
-			return xScale.options.stacked ? ruler.categoryWidth : ruler.barWidth;
+			return (xScale.options.stacked && meta.stack === undefined) ? ruler.categoryWidth : ruler.barWidth;


might be a good idea to always return bar width here and then have it correctly calculate for the stacks in getRuler

The && meta.stack === undefined retains the existing behaviour for (non-grouped) stacked bar widths - which is currently independent of barPercentage (and depends only on categoryPercentage).

I think it would be consistent to have non grouped stacked bar widths proportional to categoryPercentage AND barPercentage. If it's ok to change to make this change, I can remove meta.stack === undefined from getBarCount and remove the entire (xScale.options.stacked && meta.stack === undefined) ? ruler.categoryWidth : part from here (i.e. just use ruler.barWidth which I believe is already correctly calculated)

I think the ungrouped stacked bar (or stacked with a single group) should use the bar width. I think it's safe to go ahead and make those changes. This will likely cause some test changes as well if we are testing the bar width at all.

Done and changed unit tests.

I also changed the controller method names - getBarCount to getStackCount and getBarIndex to getStackIndex. The ruler property barCount is now stackCount. Related unit tests changed too.

etimberg · 2016-11-08T02:29:01Z

src/scales/scale.linear.js

@@ -33,16 +33,16 @@ module.exports = function(Chart) {

 				helpers.each(datasets, function(dataset, datasetIndex) {
 					var meta = chart.getDatasetMeta(datasetIndex);
-					if (valuesPerType[meta.type] === undefined) {
-						valuesPerType[meta.type] = {
+					if (valuesPerType[meta.type + meta.stack] === undefined) {


I wonder if there is a way to not have the stack leak into here

Hm... its somewhat similar to meta.type in that it's just another property to group datasets. Maybe a common meta property that factors in both type and (an optional stack) would be better? Might be too big a change though.

Can't think of anything nicer :-(

erick2014 · 2016-11-09T13:50:41Z

hey guys, when will be this branch merged?

simonbrunel · 2016-11-09T14:19:48Z

I didn't get time to review the whole code (will do asap) but I feel that the stack option is not at the right place and should live only under scales. It could be confusing since people would expect that two datasets with the same stack id would be automatically stacked (so implicitly set scale.stacked: true), whatever scale is bound to each datasets. Also, the current approach doesn't give room for future stack customization (name, style, etc.).

A rough idea would be to make the stacked option to accept a config object:

(deprecated code hidden)

stacked: true would mean to stack all datasets using that scale.

potatopeelings · 2016-11-10T16:21:13Z

@simonbrunel - the stack name feels a lot similar to the xAxisID and yAxisID properties on the dataset. And it would probably be a bit easier to manipulate the datasets in one place vs. changing them in both the data property and within the options property.

IMO stack (i.e. the group name) is more data than option (data being something you can't omit without losing information, and option being something you can omit if you're ok with the defaults).

simonbrunel · 2016-11-10T18:26:03Z

@potatopeelings, after a second thought, I agree with you that it's more convenient to keep the stack property under datasets. Then, what about to implicitly turn stacking on if scale.stacked is undefined (and stack only datasets with an explicit stack ID)? I'm sure it would save some questions/issues.

potatopeelings · 2016-11-11T00:27:15Z

@simonbrunel - sounds good! I'll add in something in scale initialize to set stacked based on the datasets and a stack === undefined checks in the bar controller. Cheers!

simonbrunel · 2016-11-11T09:29:11Z

@etimberg what do you think about this scale.stacked behavior when undefined?

Actually, there are many cases to think about:

(a) datasets: [{}, {}, {}, {}]
(b) datasets: [{ stack: 'foo' }, { stack: 'foo' }, {}, {}]
(c) datasets: [{ stack: 'foo' }, { stack: 'foo' }, { stack: 'bar' }, { stack: 'bar' }]

scale.stacked: undefined
  (a) 4 stacks: [0][1][2][3]
  (b) 3 stacks: [0,1][2][3]
  (c) 2 stacks: [0,1][2,3]

scale.stacked: true
  (a) 1 stack: [0,1,2,3]
  (b,c) 2 stacks: [0,1][2,3]

scale.stacked: false
  (a,b,c) 4 stacks: [0][1][2][3]

Does it make sense? if so, would be great to have unit tests for all of these cases.

etimberg · 2016-11-11T15:39:52Z

@simonbrunel I think I agree with that behaviour. I think it's pretty unlikely that scale.stacked would be undefined since I think the default config has it as false.

potatopeelings · 2016-11-12T13:36:48Z

@simonbrunel - Done!

@etimberg - The default config doesn't define stacked.

BTW, there seems to be an existing (i.e. before groups) problem with stacks and logarithmic scales. I have raised an issue for it (#3585) . Just thought I'd mention it in case you notice something funny there when reviewing this PR.

simonbrunel · 2016-11-20T15:50:25Z

What's the final decision on the option name: stack or group? If we choose group, isn't there any risk to face a case where we want both at the same time but with different meaning? For example, stack for stacking (obviously) and group for tooltips or highlight.

I like group because it can apply to different chart types, but I think I'm more confident with stack regarding API evolution.

potatopeelings · 2016-11-20T22:59:46Z

I prefer group because of the same reason (applying to different chart types).

If we want to group for tooltips or highlighting, I think think something more specific would be better (tooltipGroups) or something where it's clear from the object structure like

       ...
       tooltips: {
            groups: [
                 [ "Dataset 1", "Dataset 2" ], 
                 [ "Dataset 3" ], 
            ]
       }
       ...

That said, I don't mind stack either if everyone is for that.

etimberg · 2016-12-01T23:46:07Z

@simonbrunel is this good to go?

simonbrunel · 2016-12-02T09:19:02Z

If everyone agree to use stack as config name, then I guess it's ready.

potatopeelings · 2016-12-02T09:23:58Z

I'm good with stack.

etimberg · 2016-12-03T20:20:40Z

@potatopeelings can you fix the merge conflict? then I'll merge this

potatopeelings · 2016-12-04T22:42:55Z

@etimberg - sure

murphomatic · 2016-12-09T21:48:30Z

Excited for this merge ... waiting on bated breath here. :)

potatopeelings · 2016-12-11T22:36:30Z

@etimberg - I will need a few more days to get this sorted out (nothing to do with the code - my monitor just gave out :-(). Sorry! I should have done it straightaway instead of putting it off for the weekend.

etimberg · 2016-12-11T22:52:22Z

@potatopeelings thanks for letting me know

nhaberl · 2016-12-22T14:10:21Z

Excited for this merge :-)

potatopeelings · 2017-01-01T04:05:33Z

All done!

The (new and existing) codeclimate issues are because of the similarity in code between bar and horizontalBar (only the axes differ in these functions). These can be fixed by parameterizing the axes (but anyway, it would be cleaner to do that in a separate PR)

Monitor is still wonky though - it gives out every 15 minutes instead of 1 now, so 2017 is better than 2016 :-)

etimberg · 2017-01-01T14:36:13Z

Thanks @potatopeelings

ryanwinchester · 2017-03-10T21:04:32Z

@potatopeelings is it possible to add labels to stacks like in this fiddle (uses v2.1 and I think was a previous fiddle of yours)?

http://jsfiddle.net/ryanwinchester/vw1wc464/1/

I tried editing it to use 2.5 but that didn't work for me.

(@etimberg )

vaseker · 2017-03-24T12:45:41Z

After update chart.js 2.4.0 –> 2.5.0 my chart looks this

instead of this

Can anybody suggest which configuration should I set to see the old picture?

Example page
Source code

Thanks.

simonbrunel · 2017-03-24T12:52:38Z

@vaseker That's a bug which should be fixed by #4044

vaseker · 2017-03-24T13:06:06Z

@simonbrunel understood. Thanks!

JohnCoding94 · 2018-01-09T09:25:34Z

@simonbrunel i still have this bug with the current version, do we know why?

simonbrunel · 2018-01-09T10:21:07Z

@JohnCoding94 can you build a jsfiddle that demonstrates your issue with 2.7.1?

JohnCoding94 · 2018-01-09T10:28:21Z

@simonbrunel i am using react-chartjs-2, i'll try to post that, thanks.

simonbrunel · 2018-01-09T10:34:35Z

Sounds good, also have a look to that comment to make sure you use the correct stacked options.

JohnCoding94 · 2018-01-09T10:42:01Z

Here is my options JSON:

{
      maintainAspectRatio: false,
      legend: {
        display: false,
        position: 'left'
      },
      title: {
        display: true,
        text: '2018',
      },
      scales: {
        xAxes: [{
          stacked: true,
        }],
        yAxes: [{
          stacked: true
        }]
      }
};

Do you see something wrong with that syntax?
It may comes from the react-chartjs-2 wrapper, i'll post a jsfiddle ASAP.

simonbrunel · 2018-01-09T10:43:52Z

It looks ok and should stack in both directions, what result do you get then?

JohnCoding94 · 2018-01-09T15:16:45Z

The same as @vaseker when he upgraded from the 2.4.0 to the 2.5.0.

ghost · 2018-06-16T16:17:12Z

Any update on this one? https://jsfiddle.net/e8n4xd4z/9385/
read here also #5557

Apparently 2.4.0 was the latest version this thing worked. See above comment by @vaseker

Group stacked bar charts (chartjs#2643)

potatopeelings added 2 commits November 7, 2016 23:55

Group stacked bar charts (#2643)

5f33d28

Updated sample file title (for #2643)

f3a7ba7

Wildhoney mentioned this pull request Nov 7, 2016

Possible to group a stacked bar chart? #2643

Closed

etimberg requested changes Nov 8, 2016

View reviewed changes

Review comments, fixes and unit tests (#2643)

653d423

Documentation changes and behaviour for stacked=undefined (#2643)

94d7b43

etimberg mentioned this pull request Nov 22, 2016

Multiple stacked bars per label [ Bar charts ] #3636

Closed

etimberg approved these changes Dec 2, 2016

View reviewed changes

etimberg mentioned this pull request Dec 22, 2016

Support for Bullet charts #3735

Closed

potatopeelings added 2 commits January 1, 2017 13:29

Merge upstream (#2643)

eacc519

Fixed test values for upstream merge (#2643)

dd3a9d7

etimberg merged commit eebaa84 into chartjs:master Jan 1, 2017

simonbrunel added this to the Version 2.5 milestone Jan 14, 2017

swrobel mentioned this pull request Feb 1, 2017

Support for named stacks w/ Highcharts & Chart.js ankane/chartkick.js#83

Merged

etimberg mentioned this pull request Feb 1, 2017

Group bars, but don't stack them #3849

Closed

simonbrunel mentioned this pull request Mar 19, 2017

Fix and refactor bar controllers #4044

Merged

simonbrunel mentioned this pull request Jun 16, 2018

Several bars, 2 y axis, one stacked bar #5557

Closed

Cepheid-UK mentioned this pull request Oct 23, 2020

Improve Picked Hero Overview w3champions/website#216

Closed

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Group stacked bar charts (chartjs#2643) (chartjs#3563)

2d4a6a6

Group stacked bar charts (chartjs#2643)

Group stacked bar charts (#2643) #3563

Group stacked bar charts (#2643) #3563

Conversation

potatopeelings commented Nov 7, 2016

Choose a reason for hiding this comment

potatopeelings Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potatopeelings Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erick2014 commented Nov 9, 2016

simonbrunel commented Nov 9, 2016 • edited Loading

potatopeelings commented Nov 10, 2016

simonbrunel commented Nov 10, 2016

potatopeelings commented Nov 11, 2016

simonbrunel commented Nov 11, 2016 • edited Loading

etimberg commented Nov 11, 2016

potatopeelings commented Nov 12, 2016 • edited Loading

simonbrunel commented Nov 20, 2016

potatopeelings commented Nov 20, 2016

etimberg commented Dec 1, 2016

simonbrunel commented Dec 2, 2016

potatopeelings commented Dec 2, 2016

etimberg commented Dec 3, 2016

potatopeelings commented Dec 4, 2016

murphomatic commented Dec 9, 2016

potatopeelings commented Dec 11, 2016

etimberg commented Dec 11, 2016

nhaberl commented Dec 22, 2016

potatopeelings commented Jan 1, 2017

etimberg commented Jan 1, 2017

ryanwinchester commented Mar 10, 2017

vaseker commented Mar 24, 2017

simonbrunel commented Mar 24, 2017

vaseker commented Mar 24, 2017

JohnCoding94 commented Jan 9, 2018

simonbrunel commented Jan 9, 2018

JohnCoding94 commented Jan 9, 2018

simonbrunel commented Jan 9, 2018

JohnCoding94 commented Jan 9, 2018

simonbrunel commented Jan 9, 2018

JohnCoding94 commented Jan 9, 2018

ghost commented Jun 16, 2018

potatopeelings Nov 8, 2016 •

edited

Loading

potatopeelings Nov 8, 2016 •

edited

Loading

simonbrunel commented Nov 9, 2016 •

edited

Loading

simonbrunel commented Nov 11, 2016 •

edited

Loading

potatopeelings commented Nov 12, 2016 •

edited

Loading