Closed chen-ben closed 9 years ago
Oh sheit... exciting!
CC @azirbel @thangdinh @NicholasMohr might also want to take a look, as they're more familiar with the codebase
Would you please share some outcome photos of demo page (not app)? Also please try with smaller resolution screens, not always people use big screens like us.
UI Pics added! Let me know if you need a closer up screenshot
Thanks @chen-ben. It looks great. I just wonder in the 2nd pic, why do we need to show the day in the x axis label when it's in monthly interval? Shouldn't they be something like: 01/15 02/15 03/15 ... Or may look even better with Jan 15, Feb 15 ... but they takes up bigger spaces.
So I wasn't totally sure what the best way of doing it, so I just went with the way the code originally formatted months. If we exclude the year, would it possibly give too little information? I guess that is also why I changed the hours representation from showing just the hour (hh) to showing the hour minutes and seconds (hh : mm : ss). I do agree that omitting the year will make it look cleaner and make each label shorter.
@hanakimis
I am pretty sure people are going to want to be able to configure this at some point, but we could choose a single format for now, would prefer: "Jan 15" .
Thanks for the awesome work on this!
Just to be clear, I proposed Jan 15 as Jan 2015, not the 15th of Jan. (in monthly view)
woops I misunderstood before. Ok that makes sense! Should it be "Jan 15" or "Jan '15" in case the user thinks the 15 is a day and not a year?
@hanakimis Jan '15 or Jan 15 or Jan 2015? :)
"Jan 15"
bump. Any other suggestions?
Sorry, for the date format? I still prefer "Jan 15", but "Jan 2015" works for me too : )
Yup! The month labelling has already been changed so that it is the three letter representation of the month followed by the two character year.
nice thanks!
Wait... I jsut went through the comments again. I think @chen-ben you're right and "Jan '15" might be clearer.
@thangdinh @NicholasMohr @hanakimis
Good enough for a lgtm?
html/css look good to me :+1:
@Dylnuge is the other person to tag, reviewing now!
loving all the comments and tests :+1: makes it much easier to review
ok fully reviewed, poke me irl when you're done with my comments and I'll take another look-great stuff :D
LGTM other than my last few comments :tada:
Thanks for all the comments!
Description
Before, the labels along the x axis were generated using the selected interval (months, years, quarters...). This can lead to issues where the selected interval is not specific enough that the time series chart only has a few labels on the x axis with not enough specificity.
This feature dynamically generates the labels along the x axis given a maxNumberOfLabels parameter that increases the specificity until there are enough labels along the x axis. For example, if the time range spans a period of 2 years, the labels should not be year based as it would result in only two labels. The specificity should be increased to be month based.
The example website was also changed so the example time series chart includes the option to play around with the dynamic x axis.
@mgreenbaum @billy-addepar @mauriziocalo @sicasli @liuyang-li
UI Pics
Chart without dynamic x axis ticked. Note how the tick interval being set to years means that the labelling is pretty sparse along the x axis.
Dynamic x axis is ticked. This disables the tick interval (which is irrelevant when a dynamic x axis is used) and it also introduces a slider to adjust the maximum number of labels along the x axis. Here, there are a max of 14 labels across the x axis so the specificity is increased to the month level to generate the labels.
The max number of labels is now set to 7. This changes the specificity to quarters and distributes those labels across the x axis.