Skip to content
This repository has been archived by the owner on Sep 1, 2022. It is now read-only.

fix: time separator is . #327

Merged
merged 4 commits into from
Apr 20, 2022
Merged

fix: time separator is . #327

merged 4 commits into from
Apr 20, 2022

Conversation

mxdvl
Copy link
Contributor

@mxdvl mxdvl commented Apr 5, 2022

What does this change?

Fixes the time separator for the withTime method.

Why?

From the Guardian and Observer style guide: T

times
1am, 6.30pm, etc; 10 o’clock last night but 10pm yesterday; half past two, a quarter to three, 10 to 11, etc; 2hr 5min 6sec, etc; for 24-hour clock, 00.47, 23.59; noon, midnight (not 12 noon, 12 midnight or 12am, 12pm).

@mxdvl mxdvl added the bug Something isn't working label Apr 5, 2022
@mxdvl mxdvl requested a review from a team as a code owner April 5, 2022 08:32
mxdvl added 3 commits April 5, 2022 10:07
From [the Guardian and Observer style guide: T][T]

> times
> 1am, 6.30pm, etc; 10 o’clock last night but 10pm yesterday; half past two, a quarter to three, 10 to 11, etc; 2hr 5min 6sec, etc; for 24-hour clock, 00.47, 23.59; noon, midnight (not 12 noon, 12 midnight or 12am, 12pm).

[T]: https://www.theguardian.com/guardian-observer-style-guide-t
@mxdvl mxdvl force-pushed the mxdvl/fix-time-separator branch from 0170e8c to 3b81429 Compare April 5, 2022 09:07
@coveralls
Copy link

coveralls commented Apr 5, 2022

Coverage Status

Coverage remained the same at 99.81% when pulling c88f75f on mxdvl/fix-time-separator into feb8346 on main.

@@ -72,7 +72,7 @@ describe('timeAgo', () => {
const yesterday = new Date(Date.UTC(2019, 10, 16, 3, 0, 0)).getTime();

expect(timeAgo(yesterday)).toBe('1d ago');
expect(timeAgo(yesterday, { verbose: true })).toBe('Yesterday 3:00');
expect(timeAgo(yesterday, { verbose: true })).toBe('Yesterday 3.00');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The style guide seems to suggest this should omit the minutes and opt for Yesterday 3

Copy link
Member

Choose a reason for hiding this comment

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

i'd be interested to know what UX think. intuitively it feels like we have multiple use cases here. a journalist writing "yesterday at 1am" seems very different to a status of "15.00". the . though def seems to fit the style guide.

Copy link
Member

@sndrs sndrs left a comment

Choose a reason for hiding this comment

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

do you want to add reference and link to the style guide at the top of the file too?

@@ -72,7 +72,7 @@ describe('timeAgo', () => {
const yesterday = new Date(Date.UTC(2019, 10, 16, 3, 0, 0)).getTime();

expect(timeAgo(yesterday)).toBe('1d ago');
expect(timeAgo(yesterday, { verbose: true })).toBe('Yesterday 3:00');
expect(timeAgo(yesterday, { verbose: true })).toBe('Yesterday 3.00');
Copy link
Member

Choose a reason for hiding this comment

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

i'd be interested to know what UX think. intuitively it feels like we have multiple use cases here. a journalist writing "yesterday at 1am" seems very different to a status of "15.00". the . though def seems to fit the style guide.

@mxdvl mxdvl requested a review from sndrs April 5, 2022 11:40
@mxdvl mxdvl merged commit af5bfa7 into main Apr 20, 2022
@mxdvl mxdvl deleted the mxdvl/fix-time-separator branch April 20, 2022 16:17
@github-actions
Copy link

🎉 This PR is included in version 4.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants