Move and rewrite time helpers #4549
Conversation
| timestamps = model.labels; | ||
| } | ||
|
|
||
| // Remove ticks outside the min/max range |
There was a problem hiding this comment.
I think it'd be slightly cleaner if this were able to be moved inside generate. That would have a few benefits like being able to revert the test change so that the test is more meaningful and not needing the comment above generate explaining that it might not do exactly what might be expected
There was a problem hiding this comment.
There are three reasons I kept that filtering outside the generate method:
- in anticipation of the new
ticks.expandoption, in which case we will keep all ticks - we need it whatever the value of
source, so avoid code duplication - to make
generateless complex (shorter methods also better)
| quarter: { | ||
| major: false, | ||
| size: 7.884e9, | ||
| steps: [1, 2, 3, 4] |
There was a problem hiding this comment.
is 4 quarters right here? In some places in UNITS, the last step of the previous unit is approximately equivalent to the first item of the next unit.
ie. 4 quarters == 1 year
however, this is not fully consistent as 5 days !== 1 week. I'm not sure what the most correct behaviour is, but I think we should try and make it consistent.
There was a problem hiding this comment.
No, 4 quarters isn't right. If you look at all the others you notice that they max out at half the possible. E.g. there are 1000ms in a second, so the max step size is 500. Similarly, we should max quarters out at 2
I would also suggest adding 15 to minute and second as it'd be very natural to plot every 15 minutes (or seconds). In fact this is a much more natural way of referring to time than the current values. If someone asks you what time it is, you'll commonly say quarter past or quarter 'til.
Days is very tricky. Firstly, week and quarter are skipped for purposes of determining majorUnit. This is because they're uncommonly used units. If we showed "week 1", "week 2", etc. on the axis that'd be very strange. Instead we jump to the far more common unit month and show "Jan", "Feb", etc. Now the really tricky thing about this is that there's an inconsistent number of days in a month. So let's say you want to divide a month into three parts. For Feb you might plot the dates Feb 1, 10, 20 (adding 9.3 and rounding to the nearest) and for March you might plot March 1, 11, 21. This inconsistency causes other bugs like the one reported in #4523. I would suggest, let's not worry about it in this PR. I plan on sending a PR with a better logic for the tick generation in order to fix #4523
There was a problem hiding this comment.
This should be equivalent to the previous code using maxSteps: 4 === steps: [1, 2, 3, 4] ,right?
There was a problem hiding this comment.
Yes, it is. I think the old code was wrong. I'm fine if you want to leave it in this PR though I might end up changing it in my PR in that case
| var i, ilen, factor; | ||
|
|
||
| if (!steps) { | ||
| return Math.ceil(range / ((capacity || 1) * milliseconds)); |
There was a problem hiding this comment.
nice job getting rid of the loop that used to be here. It took me a while to realize that this is equivalent to the old code.
Should we add a test to cover this case?
There was a problem hiding this comment.
It took me a while to figure out the correct formula :) Not sure what extra test we can add since now this only happens for unit: 'year' and I think there is already many test using years.
There was a problem hiding this comment.
Fair point, if we have a test that uses years already then we should be good to go 😄
src/scales/scale.time.js
Outdated
| } | ||
|
|
||
| module.exports = function(Chart) { | ||
| function determineStepSize(min, max, unit, capacity) { |
There was a problem hiding this comment.
Here's a jsdoc you can add to this function:
/**
* Determine how far apart to put ticks
* @param min {number} minimum tick millis
* @param max {number} maximum tick millis
* @param unit {string} the time unit the ticks are being displayed as
* @param capacity {number} the number of labels we have room to display
* @return {string} the number of ticks to skip before displaying the next one
*/
Move time helpers back into time scale, remove the `Chart.helpers.time namespace` and attempt to make the auto generation logic a bit simpler. The generate method doesn't anymore enforce min/max, the calling code needs to clamp timestamps if needed.
simonbrunel
force-pushed
the
rewrite-time-helpers
branch
from
83fe3c0
to
f0f2fda
Compare
@simonbrunel this is only complete with added implementation of the clamping as well as tests for that? |
|
@andig it is already implemented here, outside the |
Move time helpers back into time scale, remove the `Chart.helpers.time namespace` and attempt to make the auto generation logic a bit simpler. The generate method doesn't anymore enforce min/max, the calling code needs to clamp timestamps if needed.
Move time helpers back into time scale, remove the `Chart.helpers.time namespace` and attempt to make the auto generation logic a bit simpler. The generate method doesn't anymore enforce min/max, the calling code needs to clamp timestamps if needed.
Move time helpers back into time scale, remove the
Chart.helpers.time namespaceand attempt to make the auto generation logic a bit simpler. The generate method doesn't anymore enforce min/max, the calling code needs to clamp timestamps if needed.