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

include timezone with date/time #25

Closed
wants to merge 1 commit into from
Closed

include timezone with date/time #25

wants to merge 1 commit into from

Conversation

OutdatedVersion
Copy link

When I first requested from the command line I was confused as to the time zone of the time in the response. This aims to solve that by making it clear what we're talking about.

In doing this, I also set UTC as the default time zone. While this is already what the production environment is returning doing this allowed consistent behavior across different development environments, etc.

Original:
image

New behavior:
image

This makes it clear to the end user which date we are talking about instead of making it a guessing game.
@vercel
Copy link

vercel bot commented Apr 1, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/warengonzaga/covid19-tracker-cli/gzw9ffu4a
✅ Preview: https://covid19-tracker-cli-git-fork-outdatedversion-addtimezone.warengonzaga.now.sh

@warengonzaga
Copy link
Member

@OutdatedVersion interesting... let me take a look...

@warengonzaga
Copy link
Member

warengonzaga commented Apr 1, 2020

@OutdatedVersion there is a problem on your suggested change... Many people is relying on this CLI I tried to curl the preview link provided by Zeit and it looks like it follows the default time on the server not the timezone of the person who curl the tracker. May be you can specify what timezone is using by the user instead of relying on the machine where it runs.

@OutdatedVersion
Copy link
Author

OutdatedVersion commented Apr 1, 2020

That's a good point, @warengonzaga! I did change it to use the UTC time zone. Though the system is already returning a different time zone than the user (i.e. the time zone the zeit server is set to).

I was not able to find any implementation in the code base that attempts to return the the data in the user's time zone.

For example, I receive this even though it is 12:12pm here
image

@warengonzaga
Copy link
Member

@OutdatedVersion oh, now I see the issue... its 1AM here PH time but it returns 5PM weird...

@warengonzaga
Copy link
Member

@OutdatedVersion @ianvizarra this is the cause of time issues...

asof.toLocaleString()

It reads the time on the server at Zeit so that's why it sends it to all CLI users. What a mess...

@warengonzaga
Copy link
Member

@OutdatedVersion can you create an issue for this before the PR? Thank you! I'm about to sleep I'll let @ianvizarra review your PR.

@warengonzaga warengonzaga added the bug Issue Label for Bug Reports label Apr 1, 2020
@OutdatedVersion
Copy link
Author

@warengonzaga issue created as #26 🤟

@warengonzaga
Copy link
Member

@OutdatedVersion your code is looks like outdated... btw thanks for you help I'm gonna close this... kindly PR a new one with the most updated code.

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

Successfully merging this pull request may close these issues.

2 participants