Fix wrong ticks distribution for timescale #4844
Conversation
… the visible part of a chart.
|
@simonbrunel looks like this was last touched as part of #4582, so you probably know the logic best |
|
Reported issue #4845, need a test case. |
|
I'm removing the milestone since we don't know exactly what's the bug is (if it's really a bug). |
| @@ -548,8 +548,8 @@ module.exports = function(Chart) { | |||
|
|
|||
| if (timestamps.length) { | |||
| timestamps = arrayUnique(timestamps).sort(sorter); | |||
| min = Math.min(min, timestamps[0]); | |||
| max = Math.max(max, timestamps[timestamps.length - 1]); | |||
| min = min === MAX_INTEGER ? timestamps[0] : min; | |||
There was a problem hiding this comment.
I think that's wrong, if min !== MAX_INTEGER, we still want Math.min(min, timestamps[0]); because min can change line 545 to a lower value. I don't understand why these 2 lines need to be modified, @Andgoncharov can you elaborate why these changes?
There was a problem hiding this comment.
I agree about elaborating this. @Andgoncharov could you post images with and without these changes? I also agree that we need to keep the Math.min() call.
There was a problem hiding this comment.
Hi @simonbrunel, @etimberg,
I've created a fiddle https://jsfiddle.net/andgoncharov/8bgen7b7/ to display the problem. When I change options.time.min and options.time.max values, I expect that chart is displayed within specified borders and ticks are also distributed between those values. Currently, min and max values ain't taken into account if they are inside (timestamps[0], timestamps[length - 1]) interval.
When I apply my changes, the updated chart contains a number of x-axis labels according to the settings I've specified upon creation of the chart. Or may be I misunderstood something?
| @@ -548,8 +548,8 @@ module.exports = function(Chart) { | |||
|
|
|||
| if (timestamps.length) { | |||
| timestamps = arrayUnique(timestamps).sort(sorter); | |||
| min = Math.min(min, timestamps[0]); | |||
| max = Math.max(max, timestamps[timestamps.length - 1]); | |||
| min = min === MAX_INTEGER ? timestamps[0] : min; | |||
There was a problem hiding this comment.
I agree about elaborating this. @Andgoncharov could you post images with and without these changes? I also agree that we need to keep the Math.min() call.
|
@Andgoncharov: If I understand correctly, when In this case, I think the proper fix is to ensure min/max options at line 554: // Enforce limits with user min/max options
min = parse(timeOpts.min, me) || min;
max = parse(timeOpts.max, me) || max;Then I think we can change min/max initialization (line 507) for: var min = MAX_INTEGER;
var max = MIN_INTEGER;Does it work? Unit tests would certainly need to be changed since right now we don't expect |
|
Yes, @simonbrunel I agree with your suggested fix |
|
@simonbrunel, you've formulated it very clear. I did the change you've proposed and it looks correct for me. Thank you. |
|
Fixed by #4860 |
Fix wrong ticks distribution when time.min and/or time.max are beyond the visible part of a chart.
Code to reproduce:
var options = myChart.scales['timescale'].options;
options.time.min = start;
options.time.max = end;
myChart.update(0);