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

Improvements to updated date #1731

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

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Dec 12, 2023

nextstrain.org preview

Description of proposed changes

Don't render improperly formatted dates and add a relative date in the byline.

Related issue(s)

N/A

Checklist

  • Checks pass (ignore failing downstream PR workflow)
  • If making user-facing changes, add a message in CHANGELOG.md summarizing the changes in this PR
  • (to be done by a Nextstrain team member) Create preview PRs on downstream repositories.
  • Post-merge: consider extending the schema to support an exact ISO datetime (ref)

package.json Outdated Show resolved Hide resolved
src/components/info/byline.js Outdated Show resolved Hide resolved
@victorlin victorlin requested a review from a team December 12, 2023 01:22
@victorlin victorlin force-pushed the victorlin/updated-date branch from 492ea99 to 68c9bb2 Compare December 12, 2023 01:22
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-cl8xfu December 12, 2023 01:23 Inactive
@victorlin victorlin force-pushed the victorlin/updated-date branch from 68c9bb2 to f48f071 Compare December 12, 2023 19:44
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-cl8xfu December 12, 2023 19:44 Inactive
return (
<span>
{`${t("Data updated")} ${metadata.updated}. `}
{`${t("Data updated")} `}
<Moment locale={language} date={metadata.updated} fromNow />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I haven't run the code, just reading it)

How does this work for very recent dates? In augur export the updated string uses time.strftime('%Y-%m-%d') which'll use the users local time. Our schema says nothing about time zones. I presume (?) moment's also interpreting the date string in the (Auspice) users' local time zone (or is it UTC?).

So what happens if, e.g., I update a dataset right now, thus getting "2023-12-13" and someone in the US views it (the US is currently Dec 12th)... is the dataset updated "tomorrow"? Do we ignore it because it's a bad value? What about the inverse situation, would the dataset appear to me to be updated "yesterday" even if it'd been updated 5 min ago? Time zones make this hard!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be useful to create a table or similar testing different values here. From the datasets on the Auspice review app, long-ago updated datasets do look better than before, e.g. "Data updated 2 years ago" is better than a YYYY-MM-DD string!

Copy link
Member Author

@victorlin victorlin Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, and very important to consider since we have daily ncov builds.

There's really no good way to handle this with the current schema. The same thing could be said for the current implementation of displaying meta.updated as-is without any time zone info.

I think the difference is that relative dates must assume a time zone whereas displaying as-is leaves it ambiguous (though this could be communicated better), and that its effects are greater for recent dates. Continuing your example, "tomorrow" sounds much worse than "2023-12-13", but "3 years ago" zooms the perspective out to a point where time zone doesn't matter.

How about we only show relative dates if it's at least a week old?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going further, we could extend the schema to support an exact ISO datetime (e.g. 2023-12-12T21:47:18Z) which then allows proper conversion to the user's timezone.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated so that the most granular relative date is "in the last week":

const relativeDate = date > DateTime.now().minus({ weeks: 1 }) ? t("in the last week") : date.toRelativeCalendar({ locale: language });
return (
<span>
{`${t("Data updated")} ${relativeDate} (${metadata.updated}). `}
</span>
);

@victorlin victorlin force-pushed the victorlin/updated-date branch from f48f071 to c826b5a Compare December 12, 2023 23:10
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-cl8xfu December 12, 2023 23:10 Inactive
@victorlin victorlin force-pushed the victorlin/updated-date branch 2 times, most recently from 8129473 to a89abcf Compare January 8, 2025 18:13
To be used for date parsing and validation.
A bad value could show as an inaccuracy such as "Data updated today",
which shouldn't be allowed. I discovered this upon looking at the mers
example dataset provided by get-data.sh.
To us humans, "Data updated 5 years ago" is more interpretable than
"Data updated 2020-01-01". Time flies!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants