-
Notifications
You must be signed in to change notification settings - Fork 12k
Fix the issue that the topmost tick label and the bottom of the chart area are cut off with a radial scale #5848
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
Conversation
src/core/core.layouts.js
Outdated
| helpers.each(leftBoxes.concat(rightBoxes, topBoxes, bottomBoxes), getMinimumBoxSize); | ||
|
|
||
| helpers.each(chartAreaBoxes, function(box) { | ||
| box.update(maxChartAreaWidth, maxChartAreaHeight); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Chart.js/src/core/core.scale.js
Lines 482 to 483 in f6d9a39
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
… area are cut off with a radial scale
|
Rebase done |
|
I should have run unit tests on my machine before, but same issue as #5842 (Firefox only) |
|
I will fix this and verify on my Windows PC, too. |
Fix the issue that the topmost tick label and the bottom of the chart area are cut off with a radial scale.

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.roundused to calculate the center point and radiuses. This should beMath.floorotherwise the chart area becomes larger than the canvas.Chart.js 2.7.3: https://jsfiddle.net/nagix/zm8gvnrs/

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

Fixes #5761