-
Notifications
You must be signed in to change notification settings - Fork 11
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
Have the measurement unit depend on the timezone (which is based on the metadata coordinates). #111
base: master
Are you sure you want to change the base?
Conversation
metadata coordinates).
As per @StanIwan's comment on Slack, can you double check this with some locations in Canada to ensure it is using the right unit? |
@vpetersson, it seems that the libraries being used are quite smart. Thanks @StanIwan for the feedback! |
@nicomiguelino I might be terribly mistaken here, but shouldn't Toronto be in metric (C) instead of imperial units (F)? |
Thanks, @StanIwan. I did a quick research as well to supplement your comments. It seems that Canada is using Celsius. Requesting for another review, @StanIwan and @vpetersson... |
let timezones = moment.tz.zonesForCountry(country) | ||
|
||
if (country === "BS") { | ||
timezones = timezones.filter(tz => tz !== 'America/Toronto') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no. This is a slippery slope. Please find a library that can do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason we want to tie time units to timezone instead of country?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason we want to tie time units to timezone instead of country?
how are timezone and countries correlated? A country might have a lot of timezones, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, according to ChatGPT at least, there is no country that uses both. Thus mapping it to country would be fine. Yet, I want a library for this rather than a hard coded list (which can get long i guess).
Overview
units
(for the measurement unit) query param is dependent on the current location. #110Screenshots
@StanIwan & @vpetersson Here are some screenshots of the weather app (across various locations).
Toronto
Vancouver
Mock data used
Here's a snippet from my mocked
screenly.js
: