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

Fix the issue that the topmost tick label and the bottom of the chart area are cut off with a radial scale #5848

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Nov 19, 2018

When the legend is hidden in a polarArea chart, the topmost tick label is cut off. The bottom of polarArea, pie and doughnut charts is also cut off depending of the canvas size.

The first problem happens because the height of the tick label is not considered for the padding calculation of the chart. This PR adds a few lines to the radialLinear scale for giving a hint of the top padding.

The second problem is caused by Math.round used to calculate the center point and radiuses. This should be Math.floor otherwise the chart area becomes larger than the canvas.

Chart.js 2.7.3: https://jsfiddle.net/nagix/zm8gvnrs/
screenshot 2018-11-19 at 4 36 41 pm

This PR: https://jsfiddle.net/nagix/t46b127j/
screenshot 2018-11-19 at 4 37 16 pm

Fixes #5761

@@ -205,6 +205,10 @@ module.exports = {

helpers.each(leftBoxes.concat(rightBoxes, topBoxes, bottomBoxes), getMinimumBoxSize);

helpers.each(chartAreaBoxes, function(box) {
box.update(maxChartAreaWidth, maxChartAreaHeight);
Copy link
Member

Choose a reason for hiding this comment

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

scale.update() is (unfortunately) very expensive, why do we need to call this method here while we call it already multiple time during the layout? Is the one at the step 9 not enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to take the same approach as the other vertical axis - it sets the paddingTop property at the end of fit if the axis and ticks are visible, and the step 4 picks it via getPadding. For chartAreaBoxes, this is the first time for calling update.

me.paddingTop = tickFont.size / 2;
me.paddingBottom = tickFont.size / 2;

In order to get paddingTop, we need to calculate drawingArea which is based on the sizes and positions of point labels in the chart.

Copy link
Member

Choose a reason for hiding this comment

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

I understand, but I'm very worry about consequences of calling update() on scales another time because it's already an issue (#5389). @etimberg any idea?

Copy link
Member

Choose a reason for hiding this comment

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

There are risks to calling update() again. In an ideal world, I think the update algorithm would iteratively call update() on everything until the boxes either don't change or change only by a small amount.

The current algorithm tries to approximate that by calling update() twice. The reason that chartAreaBoxes were only updated once is that I thought that the size should be dependent on everything else. i.e., the chartAreaBox is whatever is left over after the other parts have run through the fitting algorithm. That may have been a bad decision.

An alternative fix might be to change the calculation on https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.radialLinear.js#L149 to include half the font height

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etimberg Thanks for your advice. I changed the calculation of largestPossibleRadius and yCenter so that update() is called only once.

simonbrunel
simonbrunel previously approved these changes Nov 26, 2018
@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 26, 2018
etimberg
etimberg previously approved these changes Nov 27, 2018
@simonbrunel
Copy link
Member

@nagix this PR needs a rebase following the merge of #5858

@nagix
Copy link
Contributor Author

nagix commented Nov 28, 2018

Rebase done

@simonbrunel simonbrunel merged commit d6ac7d8 into chartjs:master Nov 28, 2018
@nagix nagix deleted the issue-5848 branch November 28, 2018 07:54
@simonbrunel
Copy link
Member

I should have run unit tests on my machine before, but same issue as #5842 (Firefox only)

image

@nagix
Copy link
Contributor Author

nagix commented Nov 28, 2018

I will fix this and verify on my Windows PC, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants