Honour axis min/max settings #4522
Conversation
|
This |
| ticks.push(cur.valueOf()); | ||
|
|
||
| // last tick | ||
| if (helpers.isNullOrUndef(options.max)) { |
There was a problem hiding this comment.
minor question. could we always just push realMax at the end?
There was a problem hiding this comment.
You can't because cur.valueOf() > niceRange.max in this case, meaning that the last tick will be too early. Begs the question what niceRangeMax really is and where it comes from. Didn't look into this- the current change is minimally invasive.
| if (startFraction && majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday) { | ||
|
|
||
| // first tick | ||
| if (startFraction && majorUnit && !options.timeOpts.round && !options.timeOpts.isoWeekday && helpers.isNullOrUndef(options.min)) { |
There was a problem hiding this comment.
I agree that this if is dubious. I'm not sure why isoWeekday is doing things here.
There was a problem hiding this comment.
I can't quite tell what the isoWeekday option is supposed to do from the docs. Looking at the code it seems that perhaps that setting isoWeekday makes every tick a Monday. If you're manually setting each tick to a Monday then it'd make sense not to do smart alignment here, but it's quite difficult to tell what this option is. I tried to build a sample to test it out, but the time samples seem to be currently broken on master. After #4507 is merged I imagine the samples will be working again and then we can take a stab at better documenting that feature and seeing if we should remove isoWeekday here
There was a problem hiding this comment.
I'd leave it unchanged for now as removing it breaks this https://github.com/chartjs/Chart.js/blob/master/test/specs/scale.time.tests.js#L331 test. The test itself is odd as isoWeekDay should be a boolean but the tests uses numerical value.
There was a problem hiding this comment.
My understanding of isoWeekDay is that's a number representing the day (1: Monday - 7: Sunday) that should be considered the first day of the week and I guess that isoWeekDay: true is an alias to 1 (Monday).
There was a problem hiding this comment.
Then the docs are off: https://github.com/chartjs/Chart.js/blob/master/docs/axes/cartesian/time.md shows its boolean.
|
Fixed typo and corrected lower case for nested tests. Imho ready to merge. |
|
@andig this one needs a rebase |
Fix #4514