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

Fix ticks.minor and ticks.major configuration issues #6102

Merged
merged 5 commits into from Apr 2, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Mar 1, 2019

This is a fork from #5961 separating the major/minor tick config issues from the rotation and label layout issues.

Problems

  1. Can't update scale.ticks.* options at runtime because the scale class copies the minor and major ticks options from ticks.* on the initial chart creation, and it doesn’t update them on the following update calls as the values already exist ([BUG] Can't update scale.ticks.* options at runtime since 2.7.0 #4896).
  2. ticks.major.enabled is set to false by default, but ticks.major options are effective. The behavior should be consistent with the ticks.major.enabled setting.

Solutions

  1. Deprecates mergeTicksOptions and adds parseTickFontOptions to parse minor and major tick font options. Omitted options are inherited from ticks at runtime.
  2. Uses minor tick options when ticks.major.enabled is false.

Master: https://jsfiddle.net/nagix/76fLvg8u/
screen shot 2019-03-01 at 10 22 40 pm

This PR: https://jsfiddle.net/nagix/4n5rd2gz/
screen shot 2019-03-01 at 10 25 26 pm

Fixes #4896

kurkle
kurkle previously approved these changes Mar 1, 2019
etimberg
etimberg previously approved these changes Mar 1, 2019
benmccann
benmccann previously approved these changes Mar 2, 2019
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting up the PRs. It's much easier to review this way.

I noticed that mergeTicksOptions is called in the time scale. I'm not sure why that is or why it's called both there and in buildOrUpdateControllers. It doesn't seem like logic that should be specific to the time scale and can be removed.

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

nagix commented Mar 2, 2019

Regarding mergeTicksOptions, should we remove it completely in v3?

@nagix nagix dismissed stale reviews from benmccann, etimberg, and kurkle via eb4d2cd March 2, 2019 09:30
kurkle
kurkle previously approved these changes Mar 2, 2019
etimberg
etimberg previously approved these changes Mar 2, 2019
benmccann
benmccann previously approved these changes Mar 2, 2019
kurkle
kurkle previously approved these changes Mar 10, 2019
@simonbrunel simonbrunel added this to the Version 2.9 milestone Mar 12, 2019
src/core/core.scale.js Show resolved Hide resolved
src/scales/scale.time.js Show resolved Hide resolved
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nagix Looks great, anything else before merging?

@nagix
Copy link
Contributor Author

nagix commented Apr 1, 2019

No, this is ready to merge.

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.

None yet

5 participants