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

feat: ✨ add timestamps #173

Merged
merged 6 commits into from
Dec 28, 2021
Merged

feat: ✨ add timestamps #173

merged 6 commits into from
Dec 28, 2021

Conversation

jalezi
Copy link
Collaborator

@jalezi jalezi commented Dec 27, 2021

Both timestamps (doctors and institutions) are available.
ATM using only doctors.

Check both languages.

What should we do with institutions ts?

Both timestamps (doctors and institutions) are available.
ATM using only doctors.
<br />
{t('footer.lastChange')}: <strong>{lastChange()}</strong>
*/}
{t('footer.lastChange')}: <strong>{lastChange(drTs)}</strong>
Copy link
Member

Choose a reason for hiding this comment

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

Could this be changed to use i18next interpolation for better flexibility in translations?

Suggested change
{t('footer.lastChange')}: <strong>{lastChange(drTs)}</strong>
{t('footer.lastChange', { date: new Date(drTs) })}

or remove the placeholders from translations:

"lastChange": "Last data update on {{ date, %A, %B %e, %Y }}"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure. I did't know it's possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Slightly different as you suggested.

@lukarenko
Copy link
Contributor

Do not request institutions timestamp - not needed

Copy link
Contributor

@lukarenko lukarenko left a comment

Choose a reason for hiding this comment

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

Do not request institutions timestamp

const [instTs, setInstTs] = useState(null);

const drTsRequest = useFetchTs(CSV_URL.DOCTORS_TS);
const instTsRequest = useFetchTs(CSV_URL.INSTITUTIONS_TS);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would never request institutions.csv.timestamp - no need as we only need one timestamp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@lukarenko
Copy link
Contributor

@jalezi it seems that you have broken date formatting
image

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 28, 2021

ups

could'n figure out how to use react-i18next to format date
@lukarenko
Copy link
Contributor

@jalezi why do we not use i18next formatting, like on covid-19 project?
It would be also great to add time:
"updated": "Osveženo: {{ date, %A, %e. %B %Y }}",
image

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 28, 2021

@lukarenko I think @bananica tried, I have tried. Couldn't figured out. I will try again at some point. Didn't know you want time. It can be also relative like on social

relative social
.

@jalezi
Copy link
Collaborator Author

jalezi commented Dec 28, 2021

@lukarenko I guess I figured it out.

@lukarenko lukarenko merged commit 785f2ac into master Dec 28, 2021
@lukarenko lukarenko deleted the feat/add-footer-timestamps branch December 28, 2021 12:20
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.

None yet

3 participants