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

Group stacked bar charts (#2643) #3563

Merged
merged 6 commits into from
Jan 1, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/04-Bar-Chart.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@potatopeelings potatopeelings Nov 8, 2016

Choose a reason for hiding this comment

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

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

fff8fe0e-270e-11e6-8675-f2dfb0594676

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


An example data object using these attributes is shown below.

Expand Down
105 changes: 105 additions & 0 deletions samples/bar/bar-stacked-group.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
<!doctype html>
<html>

<head>
<title>Stacked Bar Chart with Groups</title>
<script src="../../dist/Chart.bundle.js"></script>
<script src="../utils.js"></script>
<style>
canvas {
-moz-user-select: none;
-webkit-user-select: none;
-ms-user-select: none;
}
</style>
</head>

<body>
<div style="width: 75%">
<canvas id="canvas"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<script>
var barChartData = {
labels: ["January", "February", "March", "April", "May", "June", "July"],
datasets: [{
label: 'Dataset 1',
backgroundColor: window.chartColors.red,
stack: 'Stack 0',
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
label: 'Dataset 2',
backgroundColor: window.chartColors.blue,
stack: 'Stack 0',
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}, {
label: 'Dataset 3',
backgroundColor: window.chartColors.green,
stack: 'Stack 1',
data: [
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor(),
randomScalingFactor()
]
}]

};
window.onload = function() {
var ctx = document.getElementById("canvas").getContext("2d");
window.myBar = new Chart(ctx, {
type: 'bar',
data: barChartData,
options: {
title:{
display:true,
text:"Chart.js Bar Chart - Stacked"
},
tooltips: {
mode: 'index',
intersect: false
},
responsive: true,
scales: {
xAxes: [{
stacked: true,
}],
yAxes: [{
stacked: true
}]
}
}
});
};

document.getElementById('randomizeData').addEventListener('click', function() {
barChartData.datasets.forEach(function(dataset, i) {
dataset.data = dataset.data.map(function() {
return randomScalingFactor();
});
});
window.myBar.update();
});
</script>
</body>

</html>
57 changes: 37 additions & 20 deletions src/controllers/controller.bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,21 +35,27 @@ module.exports = function(Chart) {
initialize: function(chart, datasetIndex) {
Chart.DatasetController.prototype.initialize.call(this, chart, datasetIndex);

var me = this;
var meta = me.getMeta();
var dataset = me.getDataset();

meta.stack = dataset.stack;
// Use this to indicate that this is a bar dataset.
this.getMeta().bar = true;
meta.bar = true;
},

// 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() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added unit test. Cheers!

var me = this;
var barCount = 0;
var stacks = [];
helpers.each(me.chart.data.datasets, function(dataset, datasetIndex) {
var meta = me.chart.getDatasetMeta(datasetIndex);
if (meta.bar && me.chart.isDatasetVisible(datasetIndex)) {
++barCount;
if (meta.bar && me.chart.isDatasetVisible(datasetIndex) &&
(meta.stack === undefined || stacks.indexOf(meta.stack) === -1)) {
stacks.push(meta.stack);
}
}, me);
return barCount;
return stacks.length;
},

update: function(reset) {
Expand Down Expand Up @@ -109,7 +115,8 @@ module.exports = function(Chart) {
for (var i = 0; i < datasetIndex; i++) {
var currentDs = datasets[i];
var currentDsMeta = chart.getDatasetMeta(i);
if (currentDsMeta.bar && currentDsMeta.yAxisID === yScale.id && chart.isDatasetVisible(i)) {
if (currentDsMeta.bar && currentDsMeta.yAxisID === yScale.id && chart.isDatasetVisible(i) &&
meta.stack === currentDsMeta.stack) {
var currentVal = Number(currentDs.data[index]);
base += value < 0 ? Math.min(currentVal, 0) : Math.max(currentVal, 0);
}
Expand Down Expand Up @@ -159,21 +166,27 @@ module.exports = function(Chart) {
},

calculateBarWidth: function(ruler) {
var xScale = this.getScaleForId(this.getMeta().xAxisID);
var me = this;
var meta = me.getMeta();
var xScale = me.getScaleForId(meta.xAxisID);
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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@potatopeelings potatopeelings Nov 8, 2016

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

},

// Get bar index from the given dataset index accounting for the fact that not all bars are visible
// Get bar index from the given dataset index accounting for stacks and the fact that not all bars are visible
getBarIndex: function(datasetIndex) {
var meta = this.getMeta();
var barIndex = 0;
var meta, j;
var dsMeta, j;
var stacks = [meta.stack];

for (j = 0; j < datasetIndex; ++j) {
meta = this.chart.getDatasetMeta(j);
if (meta.bar && this.chart.isDatasetVisible(j)) {
dsMeta = this.chart.getDatasetMeta(j);
if (dsMeta.bar && this.chart.isDatasetVisible(j) &&
(meta.stack === undefined || stacks.indexOf(dsMeta.stack) === -1)) {
stacks.push(dsMeta.stack);
++barIndex;
}
}
Expand All @@ -189,7 +202,7 @@ module.exports = function(Chart) {
var leftTick = xScale.getPixelForValue(null, index, datasetIndex, me.chart.isCombo);
leftTick -= me.chart.isCombo ? (ruler.tickWidth / 2) : 0;

if (xScale.options.stacked) {
if (xScale.options.stacked && meta.stack === undefined) {
return leftTick + (ruler.categoryWidth / 2) + ruler.categorySpacing;
}

Expand All @@ -215,7 +228,8 @@ module.exports = function(Chart) {
for (var i = 0; i < datasetIndex; i++) {
var ds = me.chart.data.datasets[i];
var dsMeta = me.chart.getDatasetMeta(i);
if (dsMeta.bar && dsMeta.yAxisID === yScale.id && me.chart.isDatasetVisible(i)) {
if (dsMeta.bar && dsMeta.yAxisID === yScale.id && me.chart.isDatasetVisible(i) &&
meta.stack === dsMeta.stack) {
var stackedVal = Number(ds.data[index]);
if (stackedVal < 0) {
sumNeg += stackedVal || 0;
Expand Down Expand Up @@ -437,7 +451,8 @@ module.exports = function(Chart) {
for (var i = 0; i < datasetIndex; i++) {
var currentDs = datasets[i];
var currentDsMeta = chart.getDatasetMeta(i);
if (currentDsMeta.bar && currentDsMeta.xAxisID === xScale.id && chart.isDatasetVisible(i)) {
if (currentDsMeta.bar && currentDsMeta.xAxisID === xScale.id && chart.isDatasetVisible(i) &&
meta.stack === currentDsMeta.stack) {
var currentVal = Number(currentDs.data[index]);
base += value < 0 ? Math.min(currentVal, 0) : Math.max(currentVal, 0);
}
Expand Down Expand Up @@ -487,11 +502,12 @@ module.exports = function(Chart) {

calculateBarHeight: function(ruler) {
var me = this;
var yScale = me.getScaleForId(me.getMeta().yAxisID);
var meta = me.getMeta();
var yScale = me.getScaleForId(meta.yAxisID);
if (yScale.options.barThickness) {
return yScale.options.barThickness;
}
return yScale.options.stacked ? ruler.categoryHeight : ruler.barHeight;
return (yScale.options.stacked && meta.stack === undefined) ? ruler.categoryHeight : ruler.barHeight;
},

calculateBarX: function(index, datasetIndex) {
Expand All @@ -508,7 +524,8 @@ module.exports = function(Chart) {
for (var i = 0; i < datasetIndex; i++) {
var ds = me.chart.data.datasets[i];
var dsMeta = me.chart.getDatasetMeta(i);
if (dsMeta.bar && dsMeta.xAxisID === xScale.id && me.chart.isDatasetVisible(i)) {
if (dsMeta.bar && dsMeta.xAxisID === xScale.id && me.chart.isDatasetVisible(i) &&
meta.stack === dsMeta.stack) {
var stackedVal = Number(ds.data[index]);
if (stackedVal < 0) {
sumNeg += stackedVal || 0;
Expand All @@ -535,7 +552,7 @@ module.exports = function(Chart) {
var topTick = yScale.getPixelForValue(null, index, datasetIndex, me.chart.isCombo);
topTick -= me.chart.isCombo ? (ruler.tickHeight / 2) : 0;

if (yScale.options.stacked) {
if (yScale.options.stacked && meta.stack === undefined) {
return topTick + (ruler.categoryHeight / 2) + ruler.categorySpacing;
}

Expand Down
8 changes: 4 additions & 4 deletions src/scales/scale.linear.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :-(

valuesPerType[meta.type + meta.stack] = {
positiveValues: [],
negativeValues: []
};
}

// Store these per type
var positiveValues = valuesPerType[meta.type].positiveValues;
var negativeValues = valuesPerType[meta.type].negativeValues;
var positiveValues = valuesPerType[meta.type + meta.stack].positiveValues;
var negativeValues = valuesPerType[meta.type + meta.stack].negativeValues;

if (chart.isDatasetVisible(datasetIndex) && IDMatches(meta)) {
helpers.each(dataset.data, function(rawValue, index) {
Expand Down
6 changes: 3 additions & 3 deletions src/scales/scale.logarithmic.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ module.exports = function(Chart) {
helpers.each(datasets, function(dataset, datasetIndex) {
var meta = chart.getDatasetMeta(datasetIndex);
if (chart.isDatasetVisible(datasetIndex) && IDMatches(meta)) {
if (valuesPerType[meta.type] === undefined) {
valuesPerType[meta.type] = [];
if (valuesPerType[meta.type + meta.stack] === undefined) {
valuesPerType[meta.type + meta.stack] = [];
}

helpers.each(dataset.data, function(rawValue, index) {
var values = valuesPerType[meta.type];
var values = valuesPerType[meta.type + meta.stack];
var value = +me.getRightValue(rawValue);
if (isNaN(value) || meta.data[index].hidden) {
return;
Expand Down