-
Notifications
You must be signed in to change notification settings - Fork 12k
Fix regression with getLabelCapacity #6300
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
if (options.offset) { | ||
scaleSize -= labelSize; | ||
} else { | ||
scaleSize = Math.min(scaleSize, chartSize - labelSize / 2) - labelSize / 2; |
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.
If there are no y-axes, full label size should be reduced (this is accounted for here) and if there are y-axes both sides (with room for label extending that direction), then no label size should be reduced, right?.
The problem is, margins are already reduced from width if there are axes there. Didn't actually check if its dependent on position also (so if left margin is reduced, but right is not, even if there is a y-axis with position: 'right'
).
So scaleSize
could end up being labelSize / 2
too small here. example
Another issue is, if there are no y-axes, but x-axes top and bottom and labels are rotated. Then labelSize
is not enough: example
I really think scale should not be using chartSize
for anything, because it does (and should) not know enough.
All that said, this works a lot better than master / #6115
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.
If there are no y-axes, full label size should be reduced (this is accounted for here) and if there are y-axes both sides (with room for label extending that direction), then no label size should be reduced, right?.
That's correct.
The problem is, margins are already reduced from width if there are axes there.
This is not true. getLabelCapacity
is first called through update
in Step 4 then called again in Step 5&6 during layout. The margins are not passed on the first call, so they don't affect the width. They are reduced from the width in fit
which is called after buildTick
, so they don't affect in the second call, either.
Edit: I meant margins for y axes are already reduced from width as you commented, but the code recognizes it from the difference between the chart width and the scale width. The calculation is the same whether a y axis is on either the left or right as long as it is only on one side.
Didn't actually check if its dependent on position also (so if left margin is reduced, but right is not, even if there is a y-axis with position: 'right').
Right, there is no direct way to know on which side a y-axis is or both, so the this PR code is simply assuming it is on either side.
So scaleSize could end up being labelSize / 2 too small here. example
I think this example gives a not bad result if you change the canvas width to 270.
Another issue is, if there are no y-axes, but x-axes top and bottom and labels are rotated. Then labelSize is not enough: example
This is actually a very difficult problem because a scale doesn't know label widths in other scales.
I really think scale should not be using chartSize for anything, because it does (and should) not know enough.
I agree that ideally it should not. This is still a compromise solution.
@@ -123,7 +124,8 @@ describe('Time scale tests', function() { | |||
|
|||
var scaleOptions = Chart.scaleService.getScaleDefaults('time'); | |||
var scale = createScale(mockData, scaleOptions); | |||
scale.update(1000, 200); | |||
scale.chart.width = 1100; | |||
scale.update(1100, 200); |
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.
Creating the scale and then changing the width and calling update
makes it harder to step through the code with a debugger because the scale gets created and then updated so the first time you hit any line of code it doesn't count. Can we pass the width into createScale
instead so it's created with the correct width from the beginning?
The right padding in a time chart may not be correctly calculated depending on the labels and canvas width.
This occurs because
getLabelCapacity
is calculating the capacity usingmargins
but it is only available in the second call (By the way,margin
is not synchronized withwidth
even in the second call as shown in the picture below). On the other hand, the scale padding is determined based on the result of the firstgetLabelCapacity
call. If there is a difference in the results of first and secondgetLabelCapacity
calls, this problem appears. This regression was introduced in #6115.This PR fixes it by using the chart width to estimate the final scale width in the first
getLabelCapacity
call. The picture below would help to understand the calculation in each case.If the chart has y axis on both the left and the right side, the result may not be accurate, but I think a label overlap won't occur at least.
Master: https://jsfiddle.net/nagix/qkuLo506/

This PR: https://jsfiddle.net/nagix/0c597p4a/
