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

Implement scale label padding #4646

Merged
merged 1 commit into from Sep 10, 2017
+81 −70
Merged

Implement scale label padding #4646

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/axes/labelling.md
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@ The scale label configuration is nested under the scale configuration in the `sc
| `fontFamily` | `String` | `"'Helvetica Neue', 'Helvetica', 'Arial', sans-serif"` | Font family for the scale title, follows CSS font-family options.
| `fontSize` | `Number` | `12` | Font size for scale title.
| `fontStyle` | `String` | `'normal'` | Font style for the scale title, follows CSS font-style options (i.e. normal, italic, oblique, initial, inherit).
| `padding` | `Number` or `Object` | `4` | Padding to apply around scale labels. Only `top` and `bottom` are implemented.

## Creating Custom Tick Formats

Expand Down
47 changes: 33 additions & 14 deletions src/core/core.scale.js
View file
Open in desktop
Expand Up @@ -36,7 +36,14 @@ defaults._set('scale', {
// actual label
labelString: '',

lineHeight: 1.2
// line height
lineHeight: 1.2,

// top/bottom padding
padding: {
top: 4,
bottom: 4
}
},

// label settings
Expand Down Expand Up @@ -391,7 +398,6 @@ module.exports = function(Chart) {

var tickFont = parseFontOptions(tickOpts);
var tickMarkLength = opts.gridLines.tickMarkLength;
var scaleLabelLineHeight = parseLineHeight(scaleLabelOpts);

// Width
if (isHorizontal) {
Expand All @@ -410,10 +416,14 @@ module.exports = function(Chart) {

// Are we showing a title for the scale?
if (scaleLabelOpts.display && display) {
var scaleLabelLineHeight = parseLineHeight(scaleLabelOpts);
var scaleLabelPadding = helpers.options.toPadding(scaleLabelOpts.padding);
var deltaHeight = scaleLabelLineHeight + scaleLabelPadding.height;

if (isHorizontal) {
minSize.height += scaleLabelLineHeight;
minSize.height += deltaHeight;
} else {
minSize.width += scaleLabelLineHeight;
minSize.width += deltaHeight;
}
}

Expand All @@ -435,16 +445,17 @@ module.exports = function(Chart) {
// TODO - improve this calculation
var labelHeight = (sinRotation * largestTextWidth)
+ (tickFont.size * tallestLabelHeightInLines)
+ (lineSpace * tallestLabelHeightInLines);
+ (lineSpace * (tallestLabelHeightInLines - 1))
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for this change? It looks equivalent to the old code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is actually equivalent but prepares for implementing tick label padding. lineSpace is only needed lines-1 times. The last lineSpace is really outer padding and could indeed be any other value.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha

+ lineSpace; // padding

minSize.height = Math.min(me.maxHeight, minSize.height + labelHeight + tickPadding);
me.ctx.font = tickFont.font;

me.ctx.font = tickFont.font;
var firstLabelWidth = computeTextSize(me.ctx, labels[0], tickFont.font);
var lastLabelWidth = computeTextSize(me.ctx, labels[labels.length - 1], tickFont.font);

// Ensure that our ticks are always inside the canvas. When rotated, ticks are right aligned which means that the right padding is dominated
// by the font height
// Ensure that our ticks are always inside the canvas. When rotated, ticks are right aligned
// which means that the right padding is dominated by the font height
if (me.labelRotation !== 0) {
me.paddingLeft = opts.position === 'bottom' ? (cosRotation * firstLabelWidth) + 3 : (cosRotation * lineSpace) + 3; // add 3 px to move away from canvas edges
me.paddingRight = opts.position === 'bottom' ? (cosRotation * lineSpace) + 3 : (cosRotation * lastLabelWidth) + 3;
Expand All @@ -453,15 +464,18 @@ module.exports = function(Chart) {
me.paddingRight = lastLabelWidth / 2 + 3;
}
} else {
// A vertical axis is more constrained by the width. Labels are the dominant factor here, so get that length first
// Account for padding

// A vertical axis is more constrained by the width. Labels are the
// dominant factor here, so get that length first and account for padding
if (tickOpts.mirror) {
largestTextWidth = 0;
} else {
largestTextWidth += tickPadding;
// use lineSpace for consistency with horizontal axis
// tickPadding is not implemented for horizontal
largestTextWidth += tickPadding + lineSpace;
}

minSize.width = Math.min(me.maxWidth, minSize.width + largestTextWidth);

me.paddingTop = tickFont.size / 2;
me.paddingBottom = tickFont.size / 2;
}
Expand Down Expand Up @@ -663,6 +677,7 @@ module.exports = function(Chart) {

var scaleLabelFontColor = helpers.valueOrDefault(scaleLabel.fontColor, globalDefaults.defaultFontColor);
var scaleLabelFont = parseFontOptions(scaleLabel);
var scaleLabelPadding = helpers.options.toPadding(scaleLabel.padding);
var labelRotationRadians = helpers.toRadians(me.labelRotation);

var itemsToDraw = [];
Expand Down Expand Up @@ -840,10 +855,14 @@ module.exports = function(Chart) {

if (isHorizontal) {
scaleLabelX = me.left + ((me.right - me.left) / 2); // midpoint of the width
scaleLabelY = options.position === 'bottom' ? me.bottom - halfLineHeight : me.top + halfLineHeight;
scaleLabelY = options.position === 'bottom'
? me.bottom - halfLineHeight - scaleLabelPadding.bottom
: me.top + halfLineHeight + scaleLabelPadding.top;
} else {
var isLeft = options.position === 'left';
scaleLabelX = isLeft ? me.left + halfLineHeight : me.right - halfLineHeight;
scaleLabelX = isLeft
? me.left + halfLineHeight + scaleLabelPadding.top
: me.right - halfLineHeight - scaleLabelPadding.top;
Copy link
Member

Choose a reason for hiding this comment

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

[nit] would put halfLineHeight + scaleLabeePadding.top in a local var for better minification since it's use 3 times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can do but it looks kinda asymetric and hard to grasp:

			var halfLineHeight = parseLineHeight(scaleLabel) / 2;
			var halfLineHeightPlusTopPadding = halfLineHeight + scaleLabelPadding.top;

			if (isHorizontal) {
				scaleLabelX = me.left + ((me.right - me.left) / 2); // midpoint of the width
				scaleLabelY = options.position === 'bottom'
					? me.bottom - halfLineHeight - scaleLabelPadding.bottom
					: me.top + halfLineHeightPlusTopPadding;
			} else {
				var isLeft = options.position === 'left';
				scaleLabelX = isLeft
					? me.left + halfLineHeightPlusTopPadding
					: me.right - halfLineHeightPlusTopPadding;
				scaleLabelY = me.top + ((me.bottom - me.top) / 2);
				rotation = isLeft ? -0.5 * Math.PI : 0.5 * Math.PI;
			}

Is this really desired?

Copy link
Member

Choose a reason for hiding this comment

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

fair enough, that's not better. Lets leave it as is

scaleLabelY = me.top + ((me.bottom - me.top) / 2);
rotation = isLeft ? -0.5 * Math.PI : 0.5 * Math.PI;
}
Expand Down
Binary file modified 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.
12 changes: 2 additions & 10 deletions test/specs/core.helpers.tests.js
View file
Open in desktop
Expand Up @@ -128,11 +128,7 @@ describe('Core helper tests', function() {
},
position: 'right',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
beginAtZero: false,
minRotation: 0,
Expand Down Expand Up @@ -170,11 +166,7 @@ describe('Core helper tests', function() {
},
position: 'left',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
beginAtZero: false,
minRotation: 0,
Expand Down
19 changes: 19 additions & 0 deletions test/specs/core.scale.tests.js
View file
Open in desktop
@@ -1,3 +1,22 @@
describe('Core.scale', function() {
describe('auto', jasmine.specsFromFixtures('core.scale'));

it('should provide default scale label options', function() {
expect(Chart.defaults.scale.scaleLabel).toEqual({
// display property
display: false,

// actual label
labelString: '',

// actual label
lineHeight: 1.2,

// top/bottom padding
padding: {
top: 4,
bottom: 4
}
});
});
});
8 changes: 4 additions & 4 deletions test/specs/core.tooltip.tests.js
View file
Open in desktop
Expand Up @@ -143,7 +143,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(263);
expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.y).toBeCloseToPixel(155);
});

Expand Down Expand Up @@ -341,7 +341,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(263);
expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.y).toBeCloseToPixel(312);
});

Expand Down Expand Up @@ -494,7 +494,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(211);
expect(tooltip._view.x).toBeCloseToPixel(214);
expect(tooltip._view.y).toBeCloseToPixel(190);
});

Expand Down Expand Up @@ -574,7 +574,7 @@ describe('Core.Tooltip', function() {
}]
}));

expect(tooltip._view.x).toBeCloseToPixel(263);
expect(tooltip._view.x).toBeCloseToPixel(266);
expect(tooltip._view.y).toBeCloseToPixel(155);
});

Expand Down
18 changes: 7 additions & 11 deletions test/specs/scale.category.tests.js
View file
Open in desktop
Expand Up @@ -30,11 +30,7 @@ describe('Category scale tests', function() {
},
position: 'bottom',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
beginAtZero: false,
minRotation: 0,
Expand Down Expand Up @@ -215,7 +211,7 @@ describe('Category scale tests', function() {
});

var xScale = chart.scales.xScale0;
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(23);
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(23 + 6); // plus lineHeight
expect(xScale.getValueForPixel(23)).toBe(0);

expect(xScale.getPixelForValue(0, 4, 0)).toBeCloseToPixel(487);
Expand All @@ -224,7 +220,7 @@ describe('Category scale tests', function() {
xScale.options.offset = true;
chart.update();

expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(69);
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(69 + 6); // plus lineHeight
expect(xScale.getValueForPixel(69)).toBe(0);

expect(xScale.getPixelForValue(0, 4, 0)).toBeCloseToPixel(441);
Expand Down Expand Up @@ -258,8 +254,8 @@ describe('Category scale tests', function() {
});

var xScale = chart.scales.xScale0;
expect(xScale.getPixelForValue('tick_1', 0, 0)).toBeCloseToPixel(23);
expect(xScale.getPixelForValue('tick_1', 1, 0)).toBeCloseToPixel(139);
expect(xScale.getPixelForValue('tick_1', 0, 0)).toBeCloseToPixel(23 + 6); // plus lineHeight
expect(xScale.getPixelForValue('tick_1', 1, 0)).toBeCloseToPixel(143);
});

it ('Should get the correct pixel for a value when horizontal and zoomed', function() {
Expand Down Expand Up @@ -293,13 +289,13 @@ describe('Category scale tests', function() {
});

var xScale = chart.scales.xScale0;
expect(xScale.getPixelForValue(0, 1, 0)).toBeCloseToPixel(23);
expect(xScale.getPixelForValue(0, 1, 0)).toBeCloseToPixel(23 + 6); // plus lineHeight
expect(xScale.getPixelForValue(0, 3, 0)).toBeCloseToPixel(496);

xScale.options.offset = true;
chart.update();

expect(xScale.getPixelForValue(0, 1, 0)).toBeCloseToPixel(102);
expect(xScale.getPixelForValue(0, 1, 0)).toBeCloseToPixel(102 + 6); // plus lineHeight
expect(xScale.getPixelForValue(0, 3, 0)).toBeCloseToPixel(417);
});

Expand Down
22 changes: 9 additions & 13 deletions test/specs/scale.linear.tests.js
View file
Open in desktop
Expand Up @@ -28,11 +28,7 @@ describe('Linear Scale', function() {
},
position: 'left',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
beginAtZero: false,
minRotation: 0,
Expand Down Expand Up @@ -695,8 +691,8 @@ describe('Linear Scale', function() {

var xScale = chart.scales.xScale0;
expect(xScale.getPixelForValue(1, 0, 0)).toBeCloseToPixel(501); // right - paddingRight
expect(xScale.getPixelForValue(-1, 0, 0)).toBeCloseToPixel(31); // left + paddingLeft
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(266); // halfway*/
expect(xScale.getPixelForValue(-1, 0, 0)).toBeCloseToPixel(31 + 6); // left + paddingLeft + lineSpace
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(266 + 6 / 2); // halfway*/

expect(xScale.getValueForPixel(501)).toBeCloseTo(1, 1e-2);
expect(xScale.getValueForPixel(31)).toBeCloseTo(-1, 1e-2);
Expand Down Expand Up @@ -754,15 +750,15 @@ describe('Linear Scale', function() {
expect(xScale.paddingBottom).toBeCloseToPixel(0);
expect(xScale.paddingLeft).toBeCloseToPixel(0);
expect(xScale.paddingRight).toBeCloseToPixel(0);
expect(xScale.width).toBeCloseToPixel(468);
expect(xScale.width).toBeCloseToPixel(468 - 6); // minus lineSpace
expect(xScale.height).toBeCloseToPixel(28);

var yScale = chart.scales.yScale0;
expect(yScale.paddingTop).toBeCloseToPixel(0);
expect(yScale.paddingBottom).toBeCloseToPixel(0);
expect(yScale.paddingLeft).toBeCloseToPixel(0);
expect(yScale.paddingRight).toBeCloseToPixel(0);
expect(yScale.width).toBeCloseToPixel(30);
expect(yScale.width).toBeCloseToPixel(30 + 6); // plus lineSpace
expect(yScale.height).toBeCloseToPixel(452);

// Extra size when scale label showing
Expand All @@ -774,15 +770,15 @@ describe('Linear Scale', function() {
expect(xScale.paddingBottom).toBeCloseToPixel(0);
expect(xScale.paddingLeft).toBeCloseToPixel(0);
expect(xScale.paddingRight).toBeCloseToPixel(0);
expect(xScale.width).toBeCloseToPixel(454);
expect(xScale.height).toBeCloseToPixel(42);
expect(xScale.width).toBeCloseToPixel(440);
expect(xScale.height).toBeCloseToPixel(50);

expect(yScale.paddingTop).toBeCloseToPixel(0);
expect(yScale.paddingBottom).toBeCloseToPixel(0);
expect(yScale.paddingLeft).toBeCloseToPixel(0);
expect(yScale.paddingRight).toBeCloseToPixel(0);
expect(yScale.width).toBeCloseToPixel(44);
expect(yScale.height).toBeCloseToPixel(438);
expect(yScale.width).toBeCloseToPixel(58);
expect(yScale.height).toBeCloseToPixel(430);
});

it('should fit correctly when display is turned off', function() {
Expand Down
12 changes: 4 additions & 8 deletions test/specs/scale.logarithmic.tests.js
View file
Open in desktop
Expand Up @@ -27,11 +27,7 @@ describe('Logarithmic Scale tests', function() {
},
position: 'left',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
beginAtZero: false,
minRotation: 0,
Expand Down Expand Up @@ -720,9 +716,9 @@ describe('Logarithmic Scale tests', function() {

var xScale = chart.scales.xScale;
expect(xScale.getPixelForValue(80, 0, 0)).toBeCloseToPixel(495); // right - paddingRight
expect(xScale.getPixelForValue(1, 0, 0)).toBeCloseToPixel(37); // left + paddingLeft
expect(xScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(278); // halfway
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(37); // 0 is invalid, put it on the left.
expect(xScale.getPixelForValue(1, 0, 0)).toBeCloseToPixel(37 + 6); // left + paddingLeft + lineSpace
expect(xScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(278 + 6 / 2); // halfway
expect(xScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(37 + 6); // 0 is invalid, put it on the left.

expect(xScale.getValueForPixel(495)).toBeCloseToPixel(80);
expect(xScale.getValueForPixel(48)).toBeCloseTo(1, 1e-4);
Expand Down
6 changes: 1 addition & 5 deletions test/specs/scale.radialLinear.tests.js
View file
Open in desktop
Expand Up @@ -40,11 +40,7 @@ describe('Test the radial linear scale', function() {
},
position: 'chartArea',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
ticks: {
backdropColor: 'rgba(255,255,255,0.75)',
backdropPaddingY: 2,
Expand Down
6 changes: 1 addition & 5 deletions test/specs/scale.time.tests.js
View file
Open in desktop
Expand Up @@ -73,11 +73,7 @@ describe('Time scale tests', function() {
},
position: 'bottom',
offset: false,
scaleLabel: {
display: false,
labelString: '',
lineHeight: 1.2
},
scaleLabel: Chart.defaults.scale.scaleLabel,
bounds: 'data',
distribution: 'linear',
ticks: {
Expand Down