Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Conversation

nagix
Copy link
Contributor

@nagix nagix commented May 26, 2019

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 using margins but it is only available in the second call (By the way, margin is not synchronized with width 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 first getLabelCapacity call. If there is a difference in the results of first and second getLabelCapacity 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.

Screen Shot 2019-05-27 at 1 00 34 AM

Master: https://jsfiddle.net/nagix/qkuLo506/
Screen Shot 2019-05-26 at 11 47 27 PM

This PR: https://jsfiddle.net/nagix/0c597p4a/
Screen Shot 2019-05-26 at 11 47 39 PM

etimberg
etimberg previously approved these changes May 26, 2019
if (options.offset) {
scaleSize -= labelSize;
} else {
scaleSize = Math.min(scaleSize, chartSize - labelSize / 2) - labelSize / 2;
Copy link
Member

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

Copy link
Contributor Author

@nagix nagix May 27, 2019

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.
Screen Shot 2019-05-27 at 5 34 32 PM

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.

@kurkle kurkle mentioned this pull request May 28, 2019
@@ -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);
Copy link
Contributor

@benmccann benmccann May 29, 2019

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?

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.

4 participants