Skip to content

Conversation

@emrysr
Copy link
Contributor

@emrysr emrysr commented Jan 28, 2019

fix issue #50
related to emoncms issue emoncms/emoncms#1060

[screenshot of timezone dropdown]
delwedd

@TrystanLea - download size difference between branches:
current master: 543.16 KB
with new timezone 767.28 KB

The data files are asynchronous and don't seem to add much delay on the usability.

@emrysr
Copy link
Contributor Author

emrysr commented Jan 28, 2019

if this is a suitable feature I'll try to implement the same feature into any visualisations or graph embed options.

@chaveiro
Copy link
Contributor

chaveiro commented Jan 28, 2019

Why not make this setting in user profile?
Is there any benefit to let the user choose the timezone per graphic?
Currently you have "Auto" for graphics TZ that means it uses the browser timezone for visualization.
You could instead let the user choose what TZ he prefer to use for view account wise.

Server timezone is different and is used when saving data to the databases.
It should be the timezone of the devices posting to the account.
If you have devices on multiple timezones posting to the same account it will not work even if you have per graphic TZ.

@pb66
Copy link
Contributor

pb66 commented Jan 28, 2019

I have to agree it should be a user account level setting. I see no reason why we cannot just use the existing users "home" timezone from the user profile, it keeps things simple. The fewer TZ's in play the better. We only really need UTC and user "home" TZ for all but the most exotic applications. The browser TZ just confuses things, predominantly because 99% of the time it's the same as the account TZ so we don't expect the quirks when the browser TZ changes.

Then if we want to go further to cater for multi-TZ accounts, then a single per dashboard setting could set the TZ for the widgets on that dashboard in one go, including embeded graphs.

@emrysr
Copy link
Contributor Author

emrysr commented Feb 1, 2019

@pb66 and @chaveiro thanks for your review of this pull request

The idea behind this was to eventually have a similar tz offset option for standard or embedded graphs. Currently, this is set to the browser's value by default as the users would usually set their machine to the correct timezone.

An issue arises when you show a graph to a user that isn't logged in (embed or publish dashboard) and the system doesn't know what timezone the original graph relates to.

@pb66 I like the idea of the timezone being set on the dashboard which would correct any graphs or widgets within it.

If nothing is selected the current browser locale timezone is used so nothing changes for most users. I could change this to default to the current logged in user's timezone, however, this would not be passed on when embedding or sharing the graph to the "public" (non logged in users without a tz preference)

I think the best thing to do if you feel this is confusing for users is to progress with adding the timezone offset to the other places (dashboards/embeded graphs) and removing this from the main graph view...

I'll add to this pull request when I've done it and you should get a notification from github as you've already commented on this pull request.

@pb66
Copy link
Contributor

pb66 commented Feb 1, 2019

I could change this to default to the current logged in user's timezone, however, this would not be passed on when embedding or sharing the graph to the "public" (non logged in users without a tz preference)

Correct, but I suggested using the account tz not the logged in users tz. They are essentially the same thing by different names.

Anything that has an apikey or user/account name can establish the userid, from there you can get the tz.

I have the longer user/acct name displayed in the emoncms navbar so we can always tell what user/acct is being viewed. It is populated even when viewing public dashboards or via read apikey with no login. I believe it would be trivial to make the user/acct tz accessible, but I do not know the emoncms architecture well enough to catch all instances that browser tz is used.

@glynhudson
Copy link
Member

I've merged this as an initial implementation, it doesn't break anything and is an improvement over nothing. Implementation can be improved upon over time.

@glynhudson glynhudson merged commit ce6dd74 into emoncms:master Apr 16, 2019
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.

4 participants