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

Fix offsetGridLine behavior with a single data point #5609

Merged
merged 1 commit into from
Nov 2, 2018
+141 −4
Merged

Fix offsetGridLine behavior with a single data point #5609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions src/core/core.scale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,15 @@ function labelsFromTicks(ticks) {
return labels;
}

function getLineValue(scale, index, offsetGridLines) {
function getPixelForGridLine(scale, index, offsetGridLines) {
var lineValue = scale.getPixelForTick(index);

if (offsetGridLines) {
if (index === 0) {
if (scale.getTicks().length === 1) {
simonbrunel marked this conversation as resolved.
Show resolved Hide resolved
lineValue -= scale.isHorizontal() ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this draws a line on top of the axis? The axis looks extra dark in your images, which further reinforces my reading of this code. Perhaps it should return null instead and then not draw the line in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it draws a line on top of the axis, but this is not a special case as you can see in this official example (the y axis looks extra dark depending on the canvas width). This can be fixed by checking if the line and the axis border are overlapping, but wouldn't it be better to open a different PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be an easy change:

var xLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (helpers.isNullOrUndef(xLineValue)) {
    return;
}

Copy link
Contributor Author

@nagix nagix Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a bit more complicated. If it returns here, not only the grid line but also the tick label will disappear as they are drawn together. That's why lineColor is set to transparent if the offset grid line is out of the range.

So, in order to avoid drawing the grid line over axis, we need to check whether each grid line is on the axis (this requires to check gridLines.drawBorder and options.position) and to set lineColor to transparent if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right you are. Ok, in that case, I agree with you that it's complicated enough that it would perhaps be better handled in another PR instead. Thanks for the clarification

Math.max(lineValue - scale.left, scale.right - lineValue) :
Math.max(lineValue - scale.top, scale.bottom - lineValue);
} else if (index === 0) {
lineValue -= (scale.getPixelForTick(1) - lineValue) / 2;
} else {
lineValue -= (lineValue - scale.getPixelForTick(index - 1)) / 2;
Expand Down Expand Up @@ -754,7 +758,7 @@ module.exports = Element.extend({
labelY = me.bottom - labelYOffset;
}

var xLineValue = getLineValue(me, index, gridLines.offsetGridLines && ticks.length > 1);
var xLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (xLineValue < me.left - epsilon) {
lineColor = 'rgba(0,0,0,0)';
}
Expand All @@ -781,7 +785,7 @@ module.exports = Element.extend({

labelX = isLeft ? me.right - labelXOffset : me.left + labelXOffset;

var yLineValue = getLineValue(me, index, gridLines.offsetGridLines && ticks.length > 1);
var yLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (yLineValue < me.top - epsilon) {
lineColor = 'rgba(0,0,0,0)';
}
Expand Down
133 changes: 133 additions & 0 deletions test/specs/core.scale.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,137 @@ describe('Core.scale', function() {
}
});
});

var gridLineTests = [{
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: false,
offset: false,
expected: [0.5, 128.5, 256.5, 384.5, 512.5]
}, {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: false,
offset: true,
expected: [51.5, 154.5, 256.5, 358.5, 461.5]
}, {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: true,
offset: false,
expected: [-63.5, 64.5, 192.5, 320.5, 448.5]
}, {
labels: ['tick1', 'tick2', 'tick3', 'tick4', 'tick5'],
offsetGridLines: true,
offset: true,
expected: [0, 103, 205.5, 307.5, 410]
}, {
labels: ['tick1'],
offsetGridLines: false,
offset: false,
expected: [0.5]
}, {
labels: ['tick1'],
offsetGridLines: false,
offset: true,
expected: [256.5]
}, {
labels: ['tick1'],
offsetGridLines: true,
offset: false,
expected: [-511.5]
}, {
labels: ['tick1'],
offsetGridLines: true,
offset: true,
expected: [0.5]
}];

gridLineTests.forEach(function(test) {
it('should get the correct pixels for ' + test.labels.length + ' gridLine(s) for the horizontal scale when offsetGridLines is ' + test.offsetGridLines + ' and offset is ' + test.offset, function() {
var chart = window.acquireChart({
type: 'line',
data: {
datasets: [{
data: []
}],
labels: test.labels
},
options: {
scales: {
xAxes: [{
id: 'xScale0',
gridLines: {
offsetGridLines: test.offsetGridLines,
drawTicks: false
},
ticks: {
display: false
},
offset: test.offset
}],
yAxes: [{
display: false
}]
},
legend: {
display: false
}
}
});

var xScale = chart.scales.xScale0;
xScale.ctx = window.createMockContext();
chart.draw();

expect(xScale.ctx.getCalls().filter(function(x) {
return x.name === 'moveTo' && x.args[1] === 0;
}).map(function(x) {
return x.args[0];
})).toEqual(test.expected);
});
});

gridLineTests.forEach(function(test) {
it('should get the correct pixels for ' + test.labels.length + ' gridLine(s) for the vertical scale when offsetGridLines is ' + test.offsetGridLines + ' and offset is ' + test.offset, function() {
var chart = window.acquireChart({
type: 'line',
data: {
datasets: [{
data: []
}],
labels: test.labels
},
options: {
scales: {
xAxes: [{
display: false
}],
yAxes: [{
type: 'category',
id: 'yScale0',
gridLines: {
offsetGridLines: test.offsetGridLines,
drawTicks: false
},
ticks: {
display: false
},
offset: test.offset
}]
},
legend: {
display: false
}
}
});

var yScale = chart.scales.yScale0;
yScale.ctx = window.createMockContext();
chart.draw();

expect(yScale.ctx.getCalls().filter(function(x) {
return x.name === 'moveTo' && x.args[0] === 0;
}).map(function(x) {
return x.args[1];
})).toEqual(test.expected);
});
});
});