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.

Sign up for GitHub

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

Jump to bottom

A better fix for stacked bar charts with log axes #4010

Merged
merged 2 commits into from
Mar 21, 2017
Merged

A better fix for stacked bar charts with log axes #4010

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
807b723
Undo fix for #3585 since it has broken the stacked bar charts when th…
etimberg Mar 11, 2017
a2a31da
When a value of 0 is requested for a vertical logarithmic axis, retur…
etimberg Mar 18, 2017
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
20 changes: 8 additions & 12 deletions src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,9 @@ module.exports = function(Chart) {
var me = this;
var meta = me.getMeta();
var yScale = me.getScaleForId(meta.yAxisID);
var base = yScale.getBaseValue();
var original = base;
var base = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure but I would tend to think that base still need to be initialized to the scale base value because when not stacked this method returns getBasePixel() which is getPixelForValue(getBaseValue()).

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, I will fix that. Good catch!

Copy link
Member

@simonbrunel simonbrunel Mar 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm refactoring the bar controller and after a few tests, var base = 0; may be the correct way to calculate the bar positions! You can try it by setting a min on the scale. But that also means return scale.getBasePixel() is wrong.

Seems 2.5 is totally broken when stacked bar and a scale min value defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

getBasePixel() will always clip to the screen, so if the min is 50 and the max is 100, it will return 50 for the base pixel even though the bar actually starts as 0. Before we had clipping implemented this was needed because otherwise the bar drew over the axis.


if ((yScale.options.stacked === true) ||
if (yScale.options.stacked === true ||
(yScale.options.stacked === undefined && meta.stack !== undefined)) {
var chart = me.chart;
var datasets = chart.data.datasets;
Expand All @@ -127,7 +126,7 @@ module.exports = function(Chart) {
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, original) : Math.max(currentVal, original);
base += value < 0 ? Math.min(currentVal, 0) : Math.max(currentVal, 0);
}
}

Expand Down Expand Up @@ -227,9 +226,8 @@ module.exports = function(Chart) {

if (yScale.options.stacked ||
(yScale.options.stacked === undefined && meta.stack !== undefined)) {
var base = yScale.getBaseValue();
var sumPos = base,
sumNeg = base;
var sumPos = 0,
sumNeg = 0;

for (var i = 0; i < datasetIndex; i++) {
var ds = me.chart.data.datasets[i];
Expand Down Expand Up @@ -418,7 +416,6 @@ module.exports = function(Chart) {
var meta = me.getMeta();
var xScale = me.getScaleForId(meta.xAxisID);
var base = xScale.getBaseValue();
var originalBase = base;

if (xScale.options.stacked ||
(xScale.options.stacked === undefined && meta.stack !== undefined)) {
Expand All @@ -432,7 +429,7 @@ module.exports = function(Chart) {
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, originalBase) : Math.max(currentVal, originalBase);
base += value < 0 ? Math.min(currentVal, 0) : Math.max(currentVal, 0);
}
}

Expand Down Expand Up @@ -512,9 +509,8 @@ module.exports = function(Chart) {

if (xScale.options.stacked ||
(xScale.options.stacked === undefined && meta.stack !== undefined)) {
var base = xScale.getBaseValue();
var sumPos = base,
sumNeg = base;
var sumPos = 0,
sumNeg = 0;

for (var i = 0; i < datasetIndex; i++) {
var ds = me.chart.data.datasets[i];
Expand Down
2 changes: 2 additions & 0 deletions src/scales/scale.logarithmic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,8 @@ module.exports = function(Chart) {
} else {
pixel = me.top + innerDimension * 0.02 + (innerDimension * 0.98/ range * (helpers.log10(newVal)-helpers.log10(me.minNotZero)));
}
} else if (newVal === 0) {
pixel = tickOpts.reverse ? me.top : me.bottom;
} else {
range = helpers.log10(me.end) - helpers.log10(start);
innerDimension = me.height;
Expand Down
77 changes: 68 additions & 9 deletions test/specs/controller.bar.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,6 +849,65 @@ describe('Bar controller tests', function() {
});
});

it('should update elements when the scales are stacked and the y axis has a user definined minimum', function() {
var chart = window.acquireChart({
type: 'bar',
data: {
datasets: [{
data: [50, 20, 10, 100],
label: 'dataset1'
}, {
data: [50, 80, 90, 0],
label: 'dataset2'
}],
labels: ['label1', 'label2', 'label3', 'label4']
},
options: {
scales: {
xAxes: [{
type: 'category'
}],
yAxes: [{
type: 'linear',
stacked: true,
ticks: {
min: 50,
max: 100
}
}]
}
}
});

var meta0 = chart.getDatasetMeta(0);

[
{b: 936, w: 83, x: 87, y: 484},
{b: 936, w: 83, x: 202, y: 755},
{b: 936, w: 83, x: 318, y: 846},
{b: 936, w: 83, x: 434, y: 32}
].forEach(function(values, i) {
expect(meta0.data[i]._model.base).toBeCloseToPixel(values.b);
expect(meta0.data[i]._model.width).toBeCloseToPixel(values.w);
expect(meta0.data[i]._model.x).toBeCloseToPixel(values.x);
expect(meta0.data[i]._model.y).toBeCloseToPixel(values.y);
});

var meta1 = chart.getDatasetMeta(1);

[
{b: 484, w: 83, x: 87, y: 32},
{b: 755, w: 83, x: 202, y: 32},
{b: 846, w: 83, x: 318, y: 32},
{b: 32, w: 83, x: 434, y: 32}
].forEach(function(values, i) {
expect(meta1.data[i]._model.base).toBeCloseToPixel(values.b);
expect(meta1.data[i]._model.width).toBeCloseToPixel(values.w);
expect(meta1.data[i]._model.x).toBeCloseToPixel(values.x);
expect(meta1.data[i]._model.y).toBeCloseToPixel(values.y);
});
});

it('should update elements when only the category scale is stacked', function() {
var chart = window.acquireChart({
type: 'bar',
Expand Down Expand Up @@ -1119,7 +1178,7 @@ describe('Bar controller tests', function() {
}],
yAxes: [{
type: 'logarithmic',
stacked: true
stacked: true,
}]
}
}
Expand All @@ -1128,10 +1187,10 @@ describe('Bar controller tests', function() {
var meta0 = chart.getDatasetMeta(0);

[
{b: 484, w: 92, x: 94, y: 379},
{b: 484, w: 92, x: 208, y: 122},
{b: 484, w: 92, x: 322, y: 379},
{b: 484, w: 92, x: 436, y: 122}
{b: 484, w: 92, x: 94, y: 484},
{b: 484, w: 92, x: 208, y: 136},
{b: 484, w: 92, x: 322, y: 484},
{b: 484, w: 92, x: 436, y: 136}
].forEach(function(values, i) {
expect(meta0.data[i]._model.base).toBeCloseToPixel(values.b);
expect(meta0.data[i]._model.width).toBeCloseToPixel(values.w);
Expand All @@ -1142,10 +1201,10 @@ describe('Bar controller tests', function() {
var meta1 = chart.getDatasetMeta(1);

[
{b: 379, w: 92, x: 94, y: 109},
{b: 122, w: 92, x: 208, y: 109},
{b: 379, w: 92, x: 322, y: 379},
{b: 122, w: 92, x: 436, y: 25}
{b: 484, w: 92, x: 94, y: 122},
{b: 136, w: 92, x: 208, y: 122},
{b: 484, w: 92, x: 322, y: 484},
{b: 136, w: 92, x: 436, y: 32}
].forEach(function(values, i) {
expect(meta1.data[i]._model.base).toBeCloseToPixel(values.b);
expect(meta1.data[i]._model.width).toBeCloseToPixel(values.w);
Expand Down
2 changes: 1 addition & 1 deletion test/specs/scale.logarithmic.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,7 @@ describe('Logarithmic Scale tests', function() {
expect(yScale.getPixelForValue(80, 0, 0)).toBeCloseToPixel(32); // top + paddingTop
expect(yScale.getPixelForValue(1, 0, 0)).toBeCloseToPixel(484); // bottom - paddingBottom
expect(yScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(246); // halfway
expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(32); // 0 is invalid. force it on top
expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(484); // 0 is invalid. force it on bottom
Copy link
Member

Choose a reason for hiding this comment

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

Is it a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the old behaviour (0 at the top) was wrong for log Y axes


expect(yScale.getValueForPixel(32)).toBeCloseTo(80, 1e-4);
expect(yScale.getValueForPixel(484)).toBeCloseTo(1, 1e-4);
Expand Down