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

Make autoskip aware of major ticks #6509

Merged
merged 5 commits into from
Oct 19, 2019
Merged

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented Sep 8, 2019

I'm reopening #6274, which was partially reviewed earlier. I had closed it because I discovered a performance regression. Current master generates only the required ticks / labels. This PR generates a tick at every unit and then runs the auto-skipper to keep only the ones that we need. This has some benefits (see the last bullet below), but caused label generation to be more expensive. However, that is now being addressed by #6508, so I feel comfortable moving forward with this PR again

This PR does the following:

  • Skip major ticks only if they can't all fit
  • If showing all major ticks, then skip minor ticks such that they're evenly distributed between major ticks
  • Remove much of the time scale tick auto generation by generating every tick and then auto-skipping instead. This is so that we don't have two different "auto" algorithms to maintain. Also, by putting it in auto-skip it is available to all scales instead of just one. Thanks @simonbrunel for this suggestion

There's a handful of tests that have been changed because they test buildTicks. We're now building more ticks and then skipping the ones we don't need. Since the tests don't take into account the auto-skipping and are looking only at the initial buildTicks step, the results include more ticks.

Interactive examples:

Closes #4600 #4612

@benmccann benmccann changed the title Move logic from tick generation to autoskip Make autoskip aware of major ticks Sep 8, 2019
@benmccann benmccann force-pushed the autoskip branch 4 times, most recently from 2b6f0ef to bbf5ce5 Compare September 8, 2019 18:00
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Just a quick look

src/scales/scale.time.js Outdated Show resolved Hide resolved
src/scales/scale.time.js Show resolved Hide resolved
src/scales/scale.time.js Show resolved Hide resolved
test/specs/scale.time.tests.js Show resolved Hide resolved
test/specs/scale.time.tests.js Show resolved Hide resolved
etimberg
etimberg previously approved these changes Oct 5, 2019
@benmccann benmccann requested a review from kurkle October 7, 2019 04:41
kurkle
kurkle previously approved these changes Oct 7, 2019
src/core/core.scale.js Outdated Show resolved Hide resolved
etimberg
etimberg previously approved these changes Oct 9, 2019
@benmccann
Copy link
Contributor Author

I just updated this PR to cache majorIndices.length as suggested by @kurkle and to rebase on top of #6508. Thanks @etimberg and @kurkle for the reviews!

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Some performance / personal preference comments.

Overall this PR seems fine, although I'm unsure how it affects performance in different scenarios.

samples/scales/time/financial.html 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
src/core/core.scale.js Outdated Show resolved Hide resolved
etimberg
etimberg previously approved these changes Oct 14, 2019
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Looks good. I think couple of new tests for improved functionality would be good.

src/core/core.scale.js Show resolved Hide resolved
src/scales/scale.time.js Outdated Show resolved Hide resolved
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* Make autoskip aware of major ticks
* Address review comments
* Fix codeclimate warning
* Add test for major and minor tick autoskipping
* Revert change for determining _majorUnit and fix sample
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time scale only aligns single point
3 participants