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

Remove autoSkip logic to always display last tick #5891

Merged
merged 1 commit into from Dec 9, 2018

Conversation

sgray
Copy link
Contributor

@sgray sgray commented Dec 5, 2018

This changes the behavior of autoSkip so that it does not force the display of the last tick. If the last tick can be displayed with equal spacing to the rest of the ticks, it will be. Otherwise, it is not.

Fixes #5707
Fixes #3277

JSFiddle with v2.7.3: http://jsfiddle.net/61thywor/
JSFiddle with this change: http://jsfiddle.net/9t7bs01j/

@simonbrunel
Copy link
Member

Thanks @sgray, I think that's the most expected behavior.

Would you be able to share a jsfiddle with your changes?

Also, can you add a unit test? I'm surprised that it didn't break existing ones

@sgray
Copy link
Contributor Author

sgray commented Dec 5, 2018

Would you be able to share a jsfiddle with your changes?

Sure thing. I've updated the description to include JSFiddle links.

Also, can you add a unit test? I'm surprised that it didn't break existing ones

👍 I will give it a shot.

@sgray
Copy link
Contributor Author

sgray commented Dec 5, 2018

I've pushed an update that includes tests. Please take a look.

src/core/core.scale.js Outdated Show resolved Hide resolved
test/specs/core.scale.tests.js Outdated Show resolved Hide resolved
test/specs/core.scale.tests.js Outdated Show resolved Hide resolved
This changes the behavior of `autoSkip` so that it does not force the
display of the last tick. If the last tick can be displayed with equal
spacing to the rest of the ticks, it will be. Otherwise, it is not.
@simonbrunel
Copy link
Member

Thanks @sgray.

I searched from where the current behavior comes from and found that it fixes workarounds use cases where the y scale is at the 'right' position (see #1884 and the associated PR #2017). I'm not sure anymore that we can just break it. I don't think it was the correct solution because it enforces a specific use case but now I don't know what the best way to make it work better without introducing a breaking change.

@etimberg what do you think?

@etimberg
Copy link
Member

etimberg commented Dec 8, 2018

I don't think that going back to the logic from before #2017, is a good idea without some better way of configuring which ticks are shown.

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.

I'd agree with @simonbrunel that sticking with the original behavior is preferable given the user reports that consider the current behavior to be a bug

@simonbrunel
Copy link
Member

simonbrunel commented Dec 9, 2018

The initial constrain of keeping the last tick comes indirectly from #1863 (f005176) but doesn't seem to address any reported issue, so I guess it silently landed in master based on @morleyzhi preferences / use cases. But since, issues have been raised reporting this behavior as a bug (#1884, #2249, etc.).

#2017 decided to skip the penultimate tick to fix these bugs. I'm not sure why this choice (#1884 actually recommended to skip the last one) but since, many users reported this new auto skip behavior bugged (#2560, #2964, #4104, #4576, etc.)

... without some better way of configuring which ticks are shown

Except if there is a good reason of why f005176 (@morleyzhi?), I would consider this commit as a bug that no one actually wanted and would merge this PR as a bug fix (and not a breaking change). I don't think introducing a new option to workaround a fix of a wrong feature is the solution.

@benmccann you approved this PR but commented that you agree with @etimberg, what's your position?

@morleyzhi
Copy link

@simonbrunel Thanks for the thorough review of the problem! I can't remember why I implemented the force-last-tick behavior. My guess is that it was for aesthetic reasons. In that case, I'd vote for removing the forcing. I can't imagine that people would be clamoring for that behavior, so it doesn't seem worth the overhead of an extra config parameter.

@benmccann
Copy link
Contributor

benmccann commented Dec 9, 2018

@simonbrunel sorry I misread @etimberg's comment. I agree with your assessment that we should consider the current behavior a bug and merge this PR.

@etimberg etimberg merged commit 69fcba0 into chartjs:master Dec 9, 2018
@sgray sgray deleted the auto-skip-last-tick branch December 10, 2018 15:25
@andy14312
Copy link

andy14312 commented Dec 27, 2018

Hi, I still see that the last tick is still being printed even though the spacing between the ticks are uneven. I'm using the 2.7.3 version and i can still see the issue which is always displaying of last tick. Any help?

@andy14312
Copy link

@simonbrunel When is the due date for this version?

@shubsengupta
Copy link

Just curious, if the lastTick being displayed is desired behaviour for a chart, is there any way to replicate that functionality in 2.8.0?

@vishnup95
Copy link

vishnup95 commented May 12, 2020

@simonbrunel @benmccann Is there a way around this for now? I would really like the last tick behavior in the chart.

@benmccann
Copy link
Contributor

You can use afterBuildTicks to add or remove ticks from the ones that get built by default.

@vishnup95
Copy link

@benmccann Really sorry to bother you again here, a pen would be really helpful.

@Josephshanks7
Copy link

How is this still an issue? It's legitimately been years people have been complaining about this. What are we as developers supposed to do to fix, or at least bandage up, this issue in our programs?

@etimberg
Copy link
Member

etimberg commented Jul 1, 2020

@Josephshanks7 unfortunately all of the core contributors, myself included, only work on this in our free time. This is a rather complex feature that isn't easy to drop in. I looked at improving the label alignment options years ago in #3233 and ran into a lot of difficulty getting the auto-skip code to function right. This is compounded by the fact that every user has their own requirements for what needs to display or not.

Implementing more configuration of skipping behaviour may require a total rethink of how the auto-skip code works. Right now we avoid doing things like measuring the length of each label since it would be very slow.

If anyone is interested in working on it, I think a good starting point is to identify all the use cases that need to be supported and then do some solutioning. I can provide some assistance with that process if needed.

@niccolofanton
Copy link

@vishnup95 @shubsengupta you can obtain the old behavior by writing your own afterBuildTicks function as @benmccann suggested, here I wrote an example, I hope this could help you!

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
This changes the behavior of `autoSkip` so that it does not force the
display of the last tick. If the last tick can be displayed with equal
spacing to the rest of the ticks, it will be. Otherwise, it is not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants