[tickers] add milliseconds granularity #777
Conversation
PierrickKoch
force-pushed
the
master
branch
from
2774071 to
0064302
Compare
and manage granularity > MINUTELY
PierrickKoch
force-pushed
the
master
branch
from
0064302 to
2648940
Compare
src/dygraph-tickers.js
Outdated
| TEN_MSLY: 3, | ||
| FIFTY_MSLY: 4, | ||
| HTH_MSLY: 5, | ||
| FHTH_MSLY: 6, |
There was a problem hiding this comment.
What does HTH mean? At first I though "hundred thousand", but that doesn't make sense here; this is just 100/500.
There was a problem hiding this comment.
My bad, you're right :)
Would HUNDRED_MSLY and FIVE_HUNDRED_MSLY be ok for you ?
There was a problem hiding this comment.
Personally, I think there's no harm in spelling out "millisecond" instead of just using "ms". But, it's not my code 😉 (Also, as I mentioned in #776 I think it'd be interesting to explore alternative approaches that make this more customizable.)
danvk
left a comment
danvk
left a comment
There was a problem hiding this comment.
Thanks for making this change! I'd like to accept but will need to see a unit test (in auto_tests).
| assert.deepEqual([{"v":1307908020000,"label":"19:47"},{"v":1307908050000,"label":"19:47:30"},{"v":1307908080000,"label":"19:48"},{"v":1307908110000,"label":"19:48:30"},{"v":1307908140000,"label":"19:49"},{"v":1307908170000,"label":"19:49:30"}], ticker(1307908000112, 1307908172259, 800, options)); | ||
| assert.deepEqual([{"v":1307908020000,"label":"19:47"},{"v":1307908050000,"label":"19:47:30"},{"v":1307908080000,"label":"19:48"},{"v":1307908110000,"label":"19:48:30"},{"v":1307908140000,"label":"19:49"},{"v":1307908170000,"label":"19:49:30"}], ticker(1307908000112, 1307908173260, 800, options)); | ||
| assert.deepEqual([{"v":978307200000,"label":"Jan 2001"},{"v":986083200000,"label":"Apr 2001"},{"v":993945600000,"label":"Jul 2001"},{"v":1001894400000,"label":"Oct 2001"}], ticker(978307200000, 1001894400000, 400, options)); | ||
| // TODO add millisecond test ? |
There was a problem hiding this comment.
@danvk if I write a test for this, would that get this over the line for a merge?
| DECADAL: 20, | ||
| CENTENNIAL: 21, | ||
| NUM_GRANULARITIES: 22 | ||
| MILLISECONDLY: 0, |
There was a problem hiding this comment.
I'd prefer to avoid renumbering existing granularities. You can put MILLISECONDLY first in the list, but make its number 22.
There was a problem hiding this comment.
Is that wise? Not renumbering breaks less/greater-than relationships in total ordering… I’d go for it, especially as it’s this once only (nothing finer than ms supported in js)…
| return zeropad(day) + ' ' + SHORT_MONTH_NAMES_[month]; | ||
| } else if (granularity < DygraphTickers.Granularity.SECONDLY) { | ||
| var str = "" + millis; | ||
| return zeropad(secs) + "." + ('000'+str).substring(str.length); |
There was a problem hiding this comment.
Add an example of what this looks like.
…ions taken into account and mergeable with master.
…ions taken into account and mergeable with master.
* Add milliseconds granularity wtih changes from #777 with suggestions taken into account and mergeable with master. * Fix granularities according to original PR. * Add assertions for millisecond granularities * Add an example for labels below SECONDLY granularity
6 participants
Still needs some tests in
auto_tests/tests/date_ticker.js, and maybe cleaner display insrc/dygraph-utils.js(don't show milliseconds if granularity > MINUTELY or so).