Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for local timezone #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tantra35
Copy link

No description provided.

@gingerlime
Copy link

thanks for your contribution, @tantra35. Much appreciated. Timezone support is definitely nice to have.

I'm not sure I fully understand this change however. And more importantly - would it change the default timezone to all users to use their local time in their browser? because this might not be desirable for all users (myself included). So I'd love to add timezone support, but don't want to force any new timezone on users (other than the default that graphite returns, which I believe is UTC).

Could you explain how this works and whether or not this will have the effect?

@tantra35
Copy link
Author

For now there no parameter, that define timezone, and it is locale depend

toLocaleString(); - return time str is locale depended format
(d3.time.format(l_format))(new Date(d)) - also locale depended

It is possible to make custom parameter that allow redefine locale timezone. Also to check thats all work, its simply to launch patched version. But i check UTC and Moskow time, and on my opinion all works

@gingerlime
Copy link

I tested it on my browser, and it distorts the time. Perhaps my browser isn't set correctly, but I'm getting American dates (I normally use European date format), time with AM/PM (I prefer 24 hour format), and there's no indication of the timezone the data is displayed at, which I think is very important.

So if we were to implement timezone / localization I think it needs to be:

  1. Optional, and the default stays the same as it is now. So if someone upgrades giraffe they won't suddenly see unexpected changes to their dashboards
  2. Configurable via dashboards.js, so if someone wants to change their timezone they can do this by modifying their dashboard...

I guess it's possible to pass things over, and only override the default?

@tantra35
Copy link
Author

You mean that distorts time format? Time zone is simple thift in hours. For example if UTC time is 11:00, then MSK time is +3 = 14:00

If mean only time format then yes i aggree with you that needed aditional config variable that defines time format, or define locales, but in giraffe d3.locale doesn't defined

@gingerlime
Copy link

Yes, it's mainly about the formatting which isn't explicit, configurable or clear. The way it works now (for me at least) is that the format makes sense and it shows me GMT so I know which time zone the data is displayed for. After applying your change it switched to my local time zone but the format didn't indicate it + wasn't the format I expected or that seems to match my locals settings (I never use American dates).

But it's also about the time zone. I might be in Europe (UTC +1), but prefer my dashboard to show time in UTC...

So if you can fix the formatting, make it optional (the default time zone and format should stay the same), and allow tweaking the time zone, it would be great. Otherwise it changes things too much in an unexpected and non configurable way right now.

@tantra35
Copy link
Author

tantra35 commented Feb 2, 2016

Ok now exists 2 custom variables

graphite_timezone - to setup timezone offset, by default "Etc/UTC"
graphite_date_locale - to setup locale specific date formats, by default: EN

For "out-of-box" instalation we include huge moment-with-locales.js, but it can be customized by users, as moment(http://momentjs.com/) describes

@gingerlime
Copy link

this looks much better, thanks! I still have a few problems / questions:

  • The LT ll formats don't look so nice on the x-axis in my opinion. The numbers overlap each other when the charts are small. At the same time, I'm not sure if there's a universal format to use there... But I would probably change it to HH:mm, but not sure what should be better than ll. Also - if we're looking at wider date ranges (weeks, months), then this doesn't fit either. The Rickshaw defaults seem to work better for me at the moment. Is there a way to use a similar x-axis formatting to what Rickshaw uses by default? I didn't look at their implementation yet
  • Similarly, I'd like to make the hover format configurable (currently hard-coded to lll). That should be easy by setting a variable in dashboards.js (like you did with the timezone, so it's easy to override it).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants