Fix regression with getLabelCapacity#6300
Conversation
| if (options.offset) { | ||
| scaleSize -= labelSize; | ||
| } else { | ||
| scaleSize = Math.min(scaleSize, chartSize - labelSize / 2) - labelSize / 2; |
There was a problem hiding this comment.
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.
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.
| var scale = createScale(mockData, scaleOptions); | ||
| scale.update(1000, 200); | ||
| scale.chart.width = 1100; | ||
| scale.update(1100, 200); |
There was a problem hiding this comment.
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
getLabelCapacityis calculating the capacity usingmarginsbut it is only available in the second call (By the way,marginis not synchronized withwidtheven 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 firstgetLabelCapacitycall. If there is a difference in the results of first and secondgetLabelCapacitycalls, 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
getLabelCapacitycall. 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/
