Skip to content
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

autoSkip: properly calculate space needed by tick label #5922

Merged
merged 2 commits into from
Dec 21, 2018

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Dec 17, 2018

Pen with issue fixed: https://codepen.io/kurkle/pen/XodZJW
Pen with modified test case (it really needs that many labels now): https://codepen.io/kurkle/pen/JwKErL
5893

Updated based on conversation with @simonbrunel and excellent pen he made:
https://codepen.io/simonbrunel/pen/VqjBmx

Now also enabled for vertical axes, and fixes #5917
https://codepen.io/kurkle/pen/ZVOmep

Multiline labels (these are not centered properly, but thats another thing):
https://codepen.io/kurkle/pen/LMZMGq

Closes #5252
Fixes #5893
Fixes #5917

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

_autoSkip sees if there's room for the labels, but don't we need to calculate the rotation when drawing the labels as well? Where does that happen and can we share the same rotation logic between the two places?

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
simonbrunel
simonbrunel previously approved these changes Dec 19, 2018
@simonbrunel
Copy link
Member

@nagix the solution you suggested in #5917 is a bit different from these changes, any thoughts?

src/core/core.scale.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor

nagix commented Dec 20, 2018

I have the same feeling as @benmccann about rotation. The problem in the current calculateTickRotation is that it doesn't take into account multi-line labels. But, it may not cause critical issues as long as _autoSkip code supports multi-lines.

@nagix
Copy link
Contributor

nagix commented Dec 20, 2018

@simonbrunel I didn't notice that there was a check for maxTick for horizontal scales, so I'm fine with it if it also checks for vertical scales.

@simonbrunel
Copy link
Member

simonbrunel commented Dec 20, 2018

I may be wrong, but I don't see new issue here:

  • fit, _autoSkip and draw all use this.labelRotation, calculated in calculateTickRotation, so the same rotation logic applies everywhere (@benmccann)
  • _autoSkip doesn't compute any rotation but skips ticks based on this.labelRotation
  • _autoSkip calculations are now more accurate and will perform better in case of long and/or multiline labels (i.e. will not skip if it doesn't overlap when rotated).

I agree that calculateTickRotation would need to be fixed (or rewritten) because it also doesn't take in account the smallest tick interval when the distance between ticks is not constant (e.g. time scale). Though, these issues already exist and could be fixed in a separate PR IMO.

@kurkle
Copy link
Member Author

kurkle commented Dec 20, 2018

Rebased and applied new helpers.options._parsefont

@benmccann
Copy link
Contributor

I believe that both _autoSkip and calculateTickRotation try to ascertain how much space will be taken by a label for a given rotation (this may be true of the other methods @simonbrunel mentioned like fit as well, but I didn't check them). _autoSkip does it to see if we should skip the label and calculateTickRotation does it to see if we should rotate the label more or less. If we're going to invest into making that logic more accurate it seems like we could extract that piece into a helper and use it in both places

@simonbrunel simonbrunel merged commit 4b6e53a into chartjs:master Dec 21, 2018
@simonbrunel
Copy link
Member

Thanks @kurkle

@kurkle kurkle deleted the #5893 branch December 27, 2018 17:08
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
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.

[BUG] maxTicksLimit doesn't limit the number of ticks X Axis label not displayed when the string is long
5 participants