-
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
Improve tick generation for linear scales #5938
Conversation
I was confused by the chart for c with this PR because the first bar was not displaying at all. I realized after looking at the data that it's because the value is 0.5 and the axis starts at 0.5. Can we have the axis start at 0? That would solve the problem of the disappearing bar. It's also much clearer as to the relative values. It's frequently called out as misleading to not have axis start at 0: |
No (unfortunately). While I agree that scales should start at 0 by default (still need to be able to change that behavior to satisfy all use cases), it's currently not a bug but a feature ( |
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.
@nagix I like the new behavior, however I think the code is getting too complex. Could we try to refactor it (per my review comments) and apply the same "max ticks" logic whatever the stepSize
value?
I have simplified the code and updated the description above. |
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.
Looks much cleaner than original now. Functionality seems good.
Only almost equal getTickLimit
functions bother me.
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.
LGTM, just a minor change.
@@ -16,35 +16,41 @@ function generateTicks(generationOptions, dataRange) { | |||
// for details. | |||
|
|||
var stepSize = generationOptions.stepSize; |
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.
stepSize < 0
doesn't seems a valid value, so maybe we can simplify all these checks by clamping this value:
var stepSize = Math.max(0, generationOptions.stepSize);
var unit = stepSize || 1;
//...
@kurkle @simonbrunel Thanks for your comments. I refactored |
* Improve tick generation for linear scales * Simplify the tick generation code * Refactor getTickLimit
There are a few problems in tick generation for linear scales:
maxTicksLimit
is not set, the number of ticks can exceed11
which is the default value ofmaxTicksLimit
(see a and b in the example below)._autoSkip
so that the number of ticks stay withinmaxTicksLimit
, however tick values are not aligned to a nice number and the tick at the min value is missing (see c and d).stepSize
andmaxTicksLimit
are set,maxTicksLimit
doesn’t limit the number of ticks as reported in UsingstepSize
andmaxTicksLimit
together doesn't work. #2539. In this case,stepSize
should work as the minimum unit ofstepSize
for ticks, and it should be multiplied by a nice number (see d).maxTicksLimit
despite it doesn’t actually limit the number of ticks (see e and f)._autoSkip
but ingenerateTicks
(see g).Version 2.7.3: https://jsfiddle.net/nagix/eygnmo5b/
Master: https://jsfiddle.net/nagix/jds1c98z/
This PR improves tick generation for linear scales by adding the following logic:
getTickLimit
method,maxTicks
is calculated based not only onmaxTicksLimit
, the default limit (11
), the default spacing (40 pixels) or the tick font size but also onstepSize
.generateTicks
function,maxNumSpaces
forspacing
.stepSize
is used as a minimum unit if it is specified.numSpaces
) such that bothdataRange.min
anddataRange.max
are contained in the range.numSpaces
exceedsmaxNumSpaces
,spacing
is set to a nice number ofnumSpaces * spacing / maxNumSpaces
, which guarantees thatdataRange.min
anddataRange.max
are contained in the range.stepSize
is used as a minimum unit if it is specified.niceMin
andniceMax
are adjusted separately based onmin
,max
andstepSize
.Master + This PR: https://jsfiddle.net/nagix/7jxefc94/
Fixes #1968
Fixes #2539