-
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
Make autoSkip aware of major ticks #6274
Conversation
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.
Couple of comments. Didn't do a through review. Can we have this in a pen/fiddle to test out?
1368cd2
to
873778a
Compare
I'm trying out a new one - plnkr :-) I added a link in the description |
Thanks for testing this @kurkle! I think the issue is actually in the way that the sample was modified to chart Here's a slightly modified sample that works with |
a896fae
to
33bdf69
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 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.
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 |
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.
Just a quick note about another indexOf
. Will review in more depth later.
The tests that were changed were mostly tests for |
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 |
This does the following:
Interactive examples:
Closes #4600 #4612