Skip to content
This repository has been archived by the owner on Jun 5, 2023. It is now read-only.

feat: localisation time component, #185 #186

Merged
merged 4 commits into from
Mar 3, 2022

Conversation

SarahHouben
Copy link
Member

@SarahHouben SarahHouben commented Mar 3, 2022

fixes: #185

  • sets default locale to en-GB
  • sets default hourCycle to 24 and only sets hourCycle to 12 for locales where we want to display AM / PM

Screenshot:
Screenshot 2022-03-03 at 12 32 07 1

@SarahHouben SarahHouben requested a review from a team March 3, 2022 13:10
Copy link
Member

@diondiondion diondiondion left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 🙏

@@ -61,12 +69,12 @@ function getDateString({
const mins = parseInt(offset / ONE_MINUTE);
dateString = minutesAgoReadout(mins);
}
// Occcured today...
// Occured today...
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Occured today...
// Occurred today...

Sorry 😅

@@ -16,10 +16,18 @@ function date(d) {

function getDateString({
dateTime,
locale,
locale = 'en-GB',
Copy link
Member

@MrSwitch MrSwitch Mar 3, 2022

Choose a reason for hiding this comment

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

When new Intl.DateTimeFormat(locale, ...) is defined with locale === undefined it will imply the browser default language. Which for which ever language a user selects is still preferable IMO. Otherwise if we set a default, we then have to explicitly state the locale everywhere we use it.

Could you use navigator.language instead like...

Suggested change
locale = 'en-GB',
locale = navigator.language,

I'm assuming navigator.language is always going to be present and a string with a single locale reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks Andrew!

@SarahHouben SarahHouben requested a review from MrSwitch March 3, 2022 14:05
@SarahHouben SarahHouben merged commit 69805a3 into master Mar 3, 2022
@SarahHouben SarahHouben deleted the 185-time-component-localisation branch March 3, 2022 14:22
@5app-Machine
Copy link
Contributor

🎉 This PR is included in version 14.6.0 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localisation Time component
4 participants