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 #6274

Closed
wants to merge 4 commits into from

Conversation

benmccann
Copy link
Contributor

@benmccann benmccann commented May 15, 2019

This 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

Interactive examples:

Closes #4600 #4612

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.

Couple of comments. Didn't do a through review. Can we have this in a pen/fiddle to test out?

src/core/core.scale.js Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann benmccann force-pushed the autoskip branch 2 times, most recently from 1368cd2 to 873778a Compare May 15, 2019 23:17
@benmccann
Copy link
Contributor Author

Can we have this in a pen/fiddle to test out?

I'm trying out a new one - plnkr :-) I added a link in the description

@kurkle
Copy link
Member

kurkle commented May 16, 2019

Seems to work better than master in most cases.
week unit does not work well though:
PR:
image

Master:
image

@benmccann
Copy link
Contributor Author

Thanks for testing this @kurkle! I think the issue is actually in the way that the sample was modified to chart week data. The PR seems to handle week fine in my testing, but the sample wasn't written with that unit in mind. If you just add week to the dropdown then it does what you pictured. But that's not quite valid because it will generate major ticks in strange places since the sample uses a custom afterBuildTicks which doesn't support weeks. Also, week is a unit that isn't auto-selected, so the unit is chosen as day and the major unit as month, which also breaks the custom afterBuildTicks in this sample since it assumes the data is generated daily when the unit is day.

Here's a slightly modified sample that works with week: https://plnkr.co/edit/jg9aAZ?p=preview

@benmccann benmccann force-pushed the autoskip branch 8 times, most recently from a896fae to 33bdf69 Compare May 21, 2019 15:27
@kurkle
Copy link
Member

kurkle commented May 24, 2019

There is a regression in tick label overlapping:
image

image

Quite valid use case IMO, one year, unit=month, maxRotation=0. It does not overlap always.

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.

Too many tests were changed IMO.
If "all" ticks are generated and then skipped, then tests should be altered to check for non skipped ones and the result should be quite the same in most cases.

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 Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
test/specs/core.scale.tests.js Outdated Show resolved Hide resolved
test/specs/global.deprecations.tests.js Outdated Show resolved Hide resolved
test/specs/scale.time.tests.js Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

Thanks for the testing @kurkle. I've fixed the regression. The auto-skipper calculated to keep every 4th day, which was correct. The issue was in the way I changed the generate function in the time scale. It was generating a tick on every day, which was incorrect. Since it was a distribution: 'series' chart and the sample skipped weekends, it shouldn't have generated ticks on weekends. I've updated it to only generated ticks on days that have data points.

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 note about another indexOf. Will review in more depth later.

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

Too many tests were changed IMO. If "all" ticks are generated and then skipped, then tests should be altered to check for non skipped ones and the result should be quite the same in most cases.

The tests that were changed were mostly tests for buildTicks. In these changed tests, only the tick generation is being tested and not the auto-skipping.

@benmccann
Copy link
Contributor Author

I'm closing this temporarily. It has a performance regression since it generates more ticks. I will open a new version of this PR soon without the performance regression

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
2 participants