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

Fixes labelOffset not working for vertical axes #4249

Merged
merged 9 commits into from Jul 15, 2017
Merged

Fixes labelOffset not working for vertical axes #4249

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
efc3c58
Fixes labelOffset not working for vertical axes
suheb May 12, 2017
e9045a9
Adding test for labelOffset fix for vertical axes
suheb May 24, 2017
64a4ac4
Removes unnecessary options for test charts config
suheb May 26, 2017
33220aa
Increases tolerance to account for different font rendering
suheb May 26, 2017
5989543
Doubling the tolerance for label test
suheb May 26, 2017
eed700a
Using unicode icons instead of text for labelOffset test
suheb May 30, 2017
123193d
Using defaut tolerance value
suheb May 31, 2017
1411c83
Replacing test image
suheb Jun 1, 2017
0fa311b
Removing gridlines at zero
suheb Jul 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/core/core.scale.js
View file
Open in desktop
Expand Up @@ -619,7 +619,7 @@ module.exports = function(Chart) {

var yLineValue = me.getPixelForTick(index); // xvalues for grid lines
yLineValue += helpers.aliasPixel(lineWidth);
labelY = me.getPixelForTick(index, gridLines.offsetGridLines);
labelY = me.getPixelForTick(index, gridLines.offsetGridLines) + optionTicks.labelOffset;

tx1 = xTickStart;
tx2 = xTickEnd;
Expand Down
40 changes: 40 additions & 0 deletions test/fixtures/core.scale/label-offset-vertical-axes.json
View file
Open in desktop
@@ -0,0 +1,40 @@
{
"tolerance": "0.008",
Copy link
Member

Choose a reason for hiding this comment

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

0.008 might be too high, did you verify locally that the test fails if you temporary change "labelOffset": 25 to 20? Maybe it worths a try to 0.005, the lower the better :)

@etimberg I'm curious to know if there is a font that can be consistently rendered on all platforms/browsers that we could use for our unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

@simonbrunel not sure about a specific font. Maybe something monospace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonbrunel 0.008 is indeed too high. But the test does not fail (which it should) even at 0.004 if I change "labelOffset" from 25 to 20 for short labels like "1", "2", "3", "4".

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 what to do then :\ Unit tests must fail if labels are not correctly positioned, if it doesn't fail with short labels, simply use long labels to make the test relevant. Ideally, tolerance should be 0 (default is 0.001), so pick the lowest value (with long labels) that passes on Travis but that easily fail if labelOffset changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonbrunel I tried using long labels but then the test was failing even at high tolerance due to different font rendering on browsers/platform. So I tried replacing text with unicode shapes instead and it's working fine for me locally on firefox and chrome (macOS 10.2.3) but still failing on travis. Moreover, I can't see the build log for the latest commit. :/

"config": {
"type": "horizontalBar",
"data": {
"labels": ["Red", "Blue", "Yellow", "Green"],
"datasets": [{
"data": [12, 19, 3, 5]
}]
},
"options": {
"legend": false,
"title": false,
"scales": {
"xAxes": [{
"ticks": {
"display": false
},
"gridLines":{
"display": false
}
}],
"yAxes": [{
"ticks": {
"labelOffset": 25
},
"gridLines":{
"display": false
}
}]
}
}
},
"options": {
"canvas": {
"height": 256,
"width": 512
}
}
}
Binary file added test/fixtures/core.scale/label-offset-vertical-axes.png
View file
Open in desktop
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions test/specs/core.scale.tests.js
View file
Open in desktop
@@ -0,0 +1,3 @@
describe('Core.scale', function() {
describe('auto', jasmine.specsFromFixtures('core.scale'));
});