Implement scale label padding #4646

andig · 2017-08-11T16:25:58Z

Fixes #4505, currently missing tests (no idea how to implement those- help needed) and docs (will update asap).

I've also noticed that top/down labels seem to apply half line margin/padding, left/right axes don't- I assume that's desired?

Update I've also create a simple plugin that takes care of applying configurable padding to all "inner" axis per direction https://www.npmjs.com/package/chartjs-plugin-axispadding

Thanks @simonbrunel for the defered plugin- I've used that as skeleton.

etimberg

I think code looks ok. Is there any way to unit test this?

andig · 2017-08-12T09:13:28Z

Dunno. All I could offer right now is a screenshot with padding applied.

andig · 2017-08-16T18:00:22Z

ping @etimberg any suggestion how to implement a unit test? Would love to get this in.

etimberg · 2017-08-16T23:22:36Z

@andig there are some tests that compare screenshots of the canvas to a known image. Would those work here or would they be too finicky?

@simonbrunel any ideas?

simonbrunel

Images comparison will not work correctly since it will contain text (and so will be rendered differently on each browsers). Maybe you can test the layout box dimensions (scale top, bottom, width, height, minHeight, minWidth, etc.) and verify that the padding is correctly taken in account?

simonbrunel · 2017-08-17T07:35:02Z

docs/axes/labelling.md

@@ -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`` | `0` | Padding to apply around scale labels. Only `top` and `bottom` are implemented.


One extra ` at `Object``

simonbrunel · 2017-08-17T07:35:19Z

src/core/core.scale.js

@@ -396,10 +397,11 @@ module.exports = function(Chart) {

 			// Are we showing a title for the scale?
 			if (scaleLabelOpts.display && display) {
+				var padding = scaleLabelPadding.top + scaleLabelPadding.bottom;


var padding = scaleLabelPadding.height;

andig · 2017-08-17T16:02:34Z

@simonbrunel will make the desired changes.

Maybe you can test the layout box dimensions (scale top, bottom, width, height, minHeight, minWidth, etc.) and verify that the padding is correctly taken in account?

Any example where/howto getbthat box? From the rendered chart or maybe any of the events?

Another idea: would it make sense to get rid of the lineHeightoption and use the padding instead? Would mean defaulting padding (maybe limited to horizontal axes) to half font size split top/bottom. Downside: takes away multi-line labels but those don‘t seem supported anyway.
Reason I‘m asking is that he linehight is already implemented as hardcoded padding top/bottom padding anyway?

simonbrunel · 2017-08-17T16:23:13Z

That's weird, I thought @etimberg implemented multi-line support for the scale label.

For the unit test(s), maybe something like:

var chart = acquireChart({
	type: 'line',
	data: { ... },
	options: {
		// ...
		scales: {
			xAxes: [{
				id: 'x',
				scaleLabel: {
					padding: ...
				}
				//...
			}]
		}
	}
});

var scale = chart.scales.x;
expect(scale.top).toBe(...);
expect(scale.bottom).toBe(...);
expect(scale.minSize).toEqual({
	height: ...,
	width: ...
});

andig · 2017-08-17T18:24:34Z

That's weird, I thought @etimberg implemented multi-line support for the scale label.

Might have gotten this wrong. I was referring to https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L833

Would it be ok to replace this with default padding instead to make sure there is only one source for applying padding?

simonbrunel · 2017-08-18T08:11:05Z

I think this option has been introduced in 2.7 (@etimberg), so still time to remove it. I agree that it doesn't make sense to have it if we don't support multi-line, I agree to remove it. At some point, this option will be re-added when we will implement multi-line.

etimberg · 2017-08-18T22:31:15Z

Line height was added in #4387 to address an earlier bug that padding would also solve

andig · 2017-08-19T12:25:54Z

He're the current looks of the result.

With scale labels:

Without scale labels:

With scale labels but without ticks:

padding is now the only option to apply spacing around the scales. The only exception is if no scale label is display an additional half line height is added to the min tick dimenions to make sure axes don't stick together. Previously, this was only available for horizontal axes.

andig · 2017-08-21T08:27:17Z

@etimberg in order to advance this PR I'd need some help with the failing tests. Some are obvious (need to add the padding to the expected config, need to add the padding to the layout calculations), some are not (e.g. calculating "halfway" scale positions). It also seems a lot of effort to do these changed manually all over the place. Is this really the right way to go?

simonbrunel · 2017-08-21T08:47:46Z

src/core/core.scale.js

@@ -35,7 +35,11 @@ defaults._set('scale', {
 		// actual label
 		labelString: '',

-		lineHeight: 1.2
+		// top/bottom padding
+		padding: {


Should be 0 to avoid changing existing charts?

simonbrunel · 2017-08-21T08:59:32Z

docs/axes/labelling.md

 | `fontColor` | Color | `'#666'` | Font color for scale title.
 | `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` | `0` | Padding to apply around scale labels. Only `top` and `bottom` are implemented. The `top` direction always refers to the edge of the label that is furthest aways from the chart center, i.e. for a bottom axis this `top` padding is applied at the bottom of the label.


"The top direction always refers to the edge of the label that is furthest aways from the chart center, i.e. for a bottom axis this top padding is applied at the bottom of the label."

Seems quite confusing to me, padding should not depend on the scale position but only on the label rotation, so in case of position: bottom, padding top is the edge "closer" to the chart center.

I understand. This is however the only way to set padding as global default without having different default per axis position since the bottom axis basically has reversed orientation.Can still be overridden on per-axis basis by user.

andig · 2017-08-21T14:02:13Z

ping @simonbrunel updated as discussed:

Could you kindly take a look at the remaining test failures? One is the fixture I don't know nothing about.
The other is with stacked bars. I'd expect the bar width to get a little smaller due to y axis padding, however it only seems to fail for some bars which is weird?

simonbrunel · 2017-08-21T19:58:49Z

For the fixture test, you might need to tweak the JSON file to remove the extra spacing to obtain the same result as the image, or regenerate the PNG (details in #3988). For the other failing test, I'm not sure. We should write tests with minimal features (only the one that needs to be tested) and in most case the scale should be hidden. That would prevent this kind of changes.

andig · 2017-08-24T16:03:32Z

I will rebase this once #4694 is merged and will wait for how the updated fixture performs on travis.

simonbrunel

Thanks @andig.

@etimberg should this be in 2.7 or 2.8?

etimberg · 2017-09-02T17:12:28Z

src/core/core.scale.js

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


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

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.

etimberg · 2017-09-02T17:15:39Z

src/core/core.scale.js

-					scaleLabelX = isLeft ? me.left + halfLineHeight : me.right - halfLineHeight;
+					scaleLabelX = isLeft
+						? me.left + halfLineHeight + scaleLabelPadding.top
+						: me.right - halfLineHeight - scaleLabelPadding.top;


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

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?

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

etimberg · 2017-09-02T17:16:25Z

@simonbrunel I am ok with this in either version. it really depends on when 2.7 goes out the door

etimberg reviewed Aug 11, 2017

View reviewed changes

andig force-pushed the scale-padding branch from 3cd4729 to 569b377 Compare August 13, 2017 16:42

simonbrunel requested changes Aug 17, 2017

View reviewed changes

simonbrunel reviewed Aug 21, 2017

View reviewed changes

andig force-pushed the scale-padding branch 3 times, most recently from da9c53e to 27e14a4 Compare August 21, 2017 11:25

andig force-pushed the scale-padding branch from 3eaf8fd to 8a6d5b0 Compare August 22, 2017 09:41

andig mentioned this pull request Aug 22, 2017

Make bar tests independent from scales #4686

Merged

andig force-pushed the scale-padding branch from 5b24247 to 7df8f5f Compare August 25, 2017 08:51

andig mentioned this pull request Aug 25, 2017

Make bar tests more stable #4694

Merged

simonbrunel added the type: enhancement label Aug 25, 2017

andig mentioned this pull request Sep 1, 2017

Version 2.7.0 #4706

Merged

4 tasks

Implement scale label padding

8209259

andig force-pushed the scale-padding branch from 7df8f5f to 8209259 Compare September 1, 2017 17:11

simonbrunel approved these changes Sep 2, 2017

View reviewed changes

simonbrunel requested a review from etimberg September 2, 2017 09:11

etimberg reviewed Sep 2, 2017

View reviewed changes

etimberg approved these changes Sep 3, 2017

View reviewed changes

simonbrunel added this to the Version 2.7 milestone Sep 10, 2017

etimberg merged commit ea703a5 into chartjs:master Sep 10, 2017

simonbrunel deleted the scale-padding branch September 10, 2017 17:18

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017

Implement scale label padding (chartjs#4646)

67240bd

nagix mentioned this pull request May 23, 2019

Missing Option: Scale Padding/Margin #4628

Closed

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Implement scale label padding (chartjs#4646)

63041fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement scale label padding #4646

Implement scale label padding #4646

andig commented Aug 11, 2017 •

edited

Loading

etimberg left a comment

andig commented Aug 12, 2017

andig commented Aug 16, 2017

etimberg commented Aug 16, 2017

simonbrunel left a comment

simonbrunel Aug 17, 2017

simonbrunel Aug 17, 2017

andig commented Aug 17, 2017

simonbrunel commented Aug 17, 2017

andig commented Aug 17, 2017

simonbrunel commented Aug 18, 2017

etimberg commented Aug 18, 2017

andig commented Aug 19, 2017 •

edited

Loading

andig commented Aug 21, 2017

simonbrunel Aug 21, 2017

simonbrunel Aug 21, 2017

andig Aug 21, 2017

andig commented Aug 21, 2017

simonbrunel commented Aug 21, 2017

andig commented Aug 24, 2017

simonbrunel left a comment

etimberg Sep 2, 2017

andig Sep 3, 2017

etimberg Sep 3, 2017

etimberg Sep 2, 2017

andig Sep 3, 2017

etimberg Sep 3, 2017

etimberg commented Sep 2, 2017

Implement scale label padding #4646

Implement scale label padding #4646

Conversation

andig commented Aug 11, 2017 • edited Loading

etimberg left a comment

Choose a reason for hiding this comment

andig commented Aug 12, 2017

andig commented Aug 16, 2017

etimberg commented Aug 16, 2017

simonbrunel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andig commented Aug 17, 2017

simonbrunel commented Aug 17, 2017

andig commented Aug 17, 2017

simonbrunel commented Aug 18, 2017

etimberg commented Aug 18, 2017

andig commented Aug 19, 2017 • edited Loading

andig commented Aug 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andig commented Aug 21, 2017

simonbrunel commented Aug 21, 2017

andig commented Aug 24, 2017

simonbrunel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etimberg commented Sep 2, 2017

andig commented Aug 11, 2017 •

edited

Loading

andig commented Aug 19, 2017 •

edited

Loading