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.

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

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

Merged
merged 4 commits into from
Dec 20, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Dec 14, 2018

This PR fixes following problems.

  1. 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).
  2. 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).
  3. There are extra spaces at top and bottom when scale.display is false in a radar chart.
  4. Vertical alignment of multi-line point labels is slightly wrong in a radar chart.
  5. 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).
  6. 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/

screen shot 2018-12-15 at 8 00 42 am

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

screen shot 2018-12-15 at 8 02 22 am

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

@@ -341,7 +319,7 @@ module.exports = Element.extend({

// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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

src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Dec 15, 2018

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

5906:
5906-5914

5893:
5893-5914

(Sorry for not syncing the gifs)

@nagix
Copy link
Contributor Author

nagix commented Dec 16, 2018

@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.

@nagix
Copy link
Contributor Author

nagix commented Dec 17, 2018

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

* @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) {
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@nagix nagix Dec 18, 2018

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

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, I agree.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Dec 18, 2018
@simonbrunel
Copy link
Member

@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
Copy link
Contributor Author

nagix commented Dec 18, 2018

toFontString was moved outside the export.

etimberg
etimberg previously approved these changes Dec 18, 2018
kurkle
kurkle previously approved these changes Dec 19, 2018
src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
simonbrunel
simonbrunel previously approved these changes Dec 19, 2018
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

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

@nagix nagix dismissed stale reviews from simonbrunel, kurkle, and etimberg via 4780832 December 20, 2018 01:31
@nagix
Copy link
Contributor Author

nagix commented Dec 20, 2018

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.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Thanks @nagix

@simonbrunel simonbrunel merged commit d29ec5a into chartjs:master Dec 20, 2018
@nagix nagix mentioned this pull request Dec 20, 2018
@nagix nagix deleted the issue-5914 branch December 20, 2018 16:19
alex2wong added a commit to alex2wong/DefinitelyTyped that referenced this pull request Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment