Add scale.pointLabels.lineHeight and scale.ticks.lineHeight options by nagix · Pull Request #5914 · chartjs/Chart.js

nagix · 2018-12-14T23:38:48Z

This PR fixes following problems.

The top and bottom point labels are cut off in a radar chart ([Radar Chart] The bottom label is chopped #4759, [BUG] Google font gets cut off from Radar canvas - line-height/padding issue? #4799).
The top point label overlaps the top tick label in a radar chart (Label padding for pointLabels #4962, [BUG]: pointLabels overlap tick labels for radar chart with large font size #5280).
There are extra spaces at top and bottom when scale.display is false in a radar chart.
Vertical alignment of multi-line point labels is slightly wrong in a radar chart.
The line height of multi-line tick labels is hard-coded, and they overlap each other (Multiple multi-line data labels and vertical spacing/line-height #4292, Multi-line rotated line-height changes on upgrade from 2.4.0 to 2.5.0 #4379).
Multi-line tick labels in a x axis at the top overlap with the chart area.

The following changes are made in this PR.

Add support for the lineHeight option to scale.ticks and scale.pointLabels. Replace the hard-coded line spacing (font size * 1.5) with lineHeight. It fallbacks to the Chart.defaults.global.defaultLineHeight if undefined.
Add a check for scale.display before calling fitWithPointLabels.
Improve calculation of the top padding for the top tick label and the center point of a radar chart.
Calculate the vertical offset of tick labels based on the position of the scale, the number of lines and lineHeight.
Add extra padding for the point label at index 0 when ticks are visible.
Add helpers.options.parseFontOptions to eliminate code duplication.

Master: https://jsfiddle.net/nagix/sm2neb58/

This PR: https://jsfiddle.net/nagix/bjh3y8s9/

Note that the overlaps of tick labels (backdrop boxes) in radar charts is a different issue (#5917). I will open another PR for it.

Fixes #4292
Fixes #4379
Fixes #4759
Fixes #4799
Fixes #4962
Fixes #5280

etimberg · 2018-12-15T01:47:08Z

src/core/core.scale.js

 		// Get the width of each grid by calculating the difference
 		// between x offsets between 0 and 1.
-		var tickFont = parseFontOptions(tickOpts);
+		var tickFont = helpers.options.parseFontOptions(tickOpts, globalDefaults);


Would it make sense to capture helpers.options.parseFontOptions into a variable to help minification?

src/core/core.scale.js

simonbrunel

Thanks @nagix. I will review this PR in details after we decided about the public API of the new helpers.

src/helpers/helpers.options.js

src/core/core.scale.js

kurkle · 2018-12-15T21:28:04Z

Tested this PR on pens from couple of issues that it does not fix. Just to see how it changes the layout.

5906:

5893:

(Sorry for not syncing the gifs)

nagix · 2018-12-16T07:28:15Z

@kurkle Regarding #5906, the problem is related how to calculate the scale dimension, and I originally worked on it. But I realized that there are multiple problems around it, so I'm thinking of opening another PR for it.

src/helpers/helpers.options.js

nagix · 2018-12-17T02:35:00Z

helpers.options.toFontString, helpers.options._parseFont and unit tests were added as well as optimization for minification.

simonbrunel · 2018-12-18T08:38:41Z

src/helpers/helpers.options.js

+	 * @param {Object} font - A font object.
+	 * @return {Stringg} The CSS font string. See https://developer.mozilla.org/en-US/docs/Web/CSS/font
+	 */
+	toFontString: function(font) {


I would not expose this method to the public API until we need to, but instead I would declare it outside the export (we can test this method via _parseFont). We should also deprecate helpers.fontString, which could be moved at the end of this file and fallback to this method (similar to what's done in the canvas helpers)

helpers.fontString is called from core.tooltip. I think tooltips.{body|title|footer}Font{Family|Size|Style|Color} should be replaced with tooltips.*.font.* in v3, but _parseFont cannot be used for these options in the current version. Should we leave helpers.fontString for now?

That's fine to keep using fontString in this case, though I would still move toFontString outside the export to make it fully private for now. What do you think?

Yes, I agree.

simonbrunel · 2018-12-18T08:45:08Z

@nagix I like the refactor, and the number of fixed issues is impressive, good job!

Just a minor change about toFontString (except if you think otherwise) then it should be ready for merging.

nagix · 2018-12-18T11:42:57Z

toFontString was moved outside the export.

src/plugins/plugin.legend.js

simonbrunel

@nagix should we merge this PR? is there any of these changes that could badly impact existing projects and should be considering breaking?

nagix · 2018-12-20T01:34:09Z

I have reviewed the code again and checked all the samples, and I didn't see any issues (I found a few issues in master which is not related to this PR, though). I think it's ready to merge.

simonbrunel

Thanks @nagix

Add lineHeight and padding props to ScaleTitleOptions, based on https://www.chartjs.org/docs/latest/axes/labelling.html. related issue chartjs/Chart.js#5914 (comment)

…hartjs#5914)

Add scale.pointLabels.lineHeight and scale.ticks.lineHeight options

6535e7f

etimberg requested a review from simonbrunel December 15, 2018 01:42

etimberg reviewed Dec 15, 2018

View reviewed changes

kurkle reviewed Dec 15, 2018

View reviewed changes

src/core/core.scale.js Outdated Show resolved Hide resolved

simonbrunel requested changes Dec 15, 2018

View reviewed changes

src/helpers/helpers.options.js Outdated Show resolved Hide resolved

src/core/core.scale.js Outdated Show resolved Hide resolved

simonbrunel reviewed Dec 16, 2018

View reviewed changes

src/helpers/helpers.options.js Outdated Show resolved Hide resolved

nagix mentioned this pull request Dec 16, 2018

[BUG] maxTicksLimit doesn't limit the number of ticks #5917

Closed

Add helpers.options.toFontString and helpers.options._parseFont

6b44d39

simonbrunel reviewed Dec 18, 2018

View reviewed changes

simonbrunel added the type: bug label Dec 18, 2018

simonbrunel added this to the Version 2.8 milestone Dec 18, 2018

Move toFontString outside the export to make it fully private

b6f1d94

etimberg previously approved these changes Dec 18, 2018

View reviewed changes

kurkle previously approved these changes Dec 19, 2018

View reviewed changes

src/plugins/plugin.legend.js Outdated Show resolved Hide resolved

simonbrunel previously approved these changes Dec 19, 2018

View reviewed changes

Remove code duplication

4780832

nagix dismissed stale reviews from simonbrunel, kurkle, and etimberg via 4780832 December 20, 2018 01:31

kurkle approved these changes Dec 20, 2018

View reviewed changes

simonbrunel approved these changes Dec 20, 2018

View reviewed changes

simonbrunel merged commit d29ec5a into chartjs:master Dec 20, 2018

nagix mentioned this pull request Dec 20, 2018

PointLabels padding #5873

Closed

nagix mentioned this pull request Dec 20, 2018

autoSkip: properly calculate space needed by tick label #5922

Merged

nagix deleted the issue-5914 branch December 20, 2018 16:19

nagix mentioned this pull request Dec 26, 2018

Improve tick generation for linear scales #5938

Merged

nagix mentioned this pull request Jan 17, 2019

line chart support new-lined text and multi-byte labels? #5985

Closed

kurkle mentioned this pull request Apr 25, 2019

Polar Chart on documentation page is cut off in Chrome #6217

Closed

snyk-bot mentioned this pull request Nov 9, 2019

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.1 socialsoftware/quizzes-tutor#45

Merged

This was referenced Mar 4, 2020

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 ajesse11x/gravity#5

Open

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 qsays/ceph#4

Open

vpetersson mentioned this pull request Mar 7, 2020

[Snyk] Upgrade chart.js from 2.7.3 to 2.9.3 WoTTsecurity/api#764

Merged

This was referenced Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 MichaelKohler/reps-voting-analysis#14

Merged

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

This was referenced Apr 14, 2020

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 mc-ireiser/startbootstrap-sb-admin-2#2

Open

[Snyk] Upgrade chart.js from 2.8.0 to 2.9.3 mc-ireiser/coreui-free-vue-admin-template#5

Open

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

Add scale.pointLabels.lineHeight and scale.ticks.lineHeight options (c…

784c2dd

…hartjs#5914)

Conversation

nagix commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etimberg Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kurkle commented Dec 15, 2018

Uh oh!

nagix commented Dec 16, 2018

Uh oh!

Uh oh!

nagix commented Dec 17, 2018

Uh oh!

simonbrunel Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

nagix Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

nagix Dec 18, 2018

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Dec 18, 2018

Uh oh!

nagix commented Dec 18, 2018

Uh oh!

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

nagix commented Dec 20, 2018

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nagix commented Dec 14, 2018 •

edited

Loading

nagix Dec 18, 2018 •

edited

Loading