-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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 overlapping auto-generated ticks on time scale #6115
Conversation
Updated solution, use margins (provided here) instead of |
#5961 addresses the same issue in Chart.js/src/core/core.scale.js Lines 436 to 443 in 8925eed
|
I think it is difficult to get |
b5ffb4f
to
8b65c6c
Compare
Limited this change to |
Well, I see... this is called before |
@kurkle this PR looks fine, should we get it merged? |
This comment has been minimized.
This comment has been minimized.
@kurkle this PR will need to be rebased |
110fd7f
to
870265a
Compare
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.
Too many labels are currently being generated because padding is not being considered when generating the labels and then padding is calculated later.
The right solution to this is to probably to calculate those values together or set a maximum padding value and use that here. I don't think the solution is to use some other value in place of padding. It seems like we're just getting lucky that's working out, so I would want to see more explanation around why that's the right thing to do.
scaleMargin
is defined as:
var scaleMargin = {
left: Math.max(outerBoxSizes.left, maxPadding.left),
right: Math.max(outerBoxSizes.right, maxPadding.right),
};
maxPadding
isn't really correct because we should use the padding for this scale and not the maximum padding of all the scales. I think maybe maxPadding
is 0
at this point in the code? outerBoxSizes
is defined as helpers.options.toPadding(layoutOptions.padding);
though the default layout.padding
is 0
so it doesn't seem like that should have an effect either. The layout and tick generation/rotation code is quite convoluted and I'm afraid this is making it harder to understand
I have a branch on my machine locally that is replacing the whole autotick generation for the time scale as suggested by @simonbrunel and described in #4612. I think it would fix the issue this is addressing. I haven't been very motivated lately to open new PRs though because it seems like there's a backlog of unreviewed PRs and if I open new PRs it's just going to push my controller defaults PR further down the list and make it take longer to get it merged. In either case I still have a few more edge cases I need to fix before sending
Account for margins in
getLabelCapacity
Pen PR
Pen master
Fixes: #6109