Web: intl add a date time short formatter #39915
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with |
kimlisa
added
the
no-changelog
avatus
approved these changes
Mar 27, 2024
ravicious
reviewed
Mar 28, 2024
| ); | ||
| } catch (err) { | ||
| logger.error('dateTimeShortFormat()', err); | ||
| return 'undefined'; |
There was a problem hiding this comment.
- What errors do we expect to be thrown here? I see that other functions in this file do use
try … catch, but they also useformatfromdate-fnsand notIntl.DateTimeFormat. - In case of an error, wouldn't it be better to use some kind of method on
Dateto display something approximate instead of saying"undefined"? IfIntl.DateTimeFormat().formatcannot fail, then the error handling is not even necessary.
There was a problem hiding this comment.
- From what I see, the function throws only when
localesoroptionscontain invalid values https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat#exceptions.
If so, should we catch the error at all? It can come only from our invalid configuration, not from the user input.
| @@ -83,3 +83,19 @@ export function displayDateTime(date: Date) { | |||
| export function dateToUtc(date: Date) { | |||
| return new Date(date.getTime() + date.getTimezoneOffset() * 60 * 1000); | |||
| } | |||
|
|
|||
| export function dateTimeShortFormat(date: Date) { | |||
There was a problem hiding this comment.
It'd be nice to put an example in a JSDoc of how the formatted date is going to look like, say for en-US at least. And maybe mention that it's going to use browser and system settings to determine the actual format that's going to be used.
gzdunek
reviewed
Mar 28, 2024
Comment on lines
89
to
96
| if (isTest) { | ||
| return new Intl.DateTimeFormat('en-US', { timeStyle: 'short' }).format( | ||
| date | ||
| ); | ||
| } | ||
| return new Intl.DateTimeFormat(undefined, { timeStyle: 'short' }).format( | ||
| date | ||
| ); |
There was a problem hiding this comment.
Suggested change
| if (isTest) { | |
| return new Intl.DateTimeFormat('en-US', { timeStyle: 'short' }).format( | |
| date | |
| ); | |
| } | |
| return new Intl.DateTimeFormat(undefined, { timeStyle: 'short' }).format( | |
| date | |
| ); | |
| const DEFAULT_LOCALE = 'en-US'; //(as const on top of the file) | |
| ... | |
| const locale = isTest ? DEFAULT_LOCALE : undefined; | |
| return new Intl.DateTimeFormat(locale, { timeStyle: 'short' }).format(date); |
| ); | ||
| } catch (err) { | ||
| logger.error('dateTimeShortFormat()', err); | ||
| return 'undefined'; |
There was a problem hiding this comment.
- From what I see, the function throws only when
localesoroptionscontain invalid values https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/DateTimeFormat/DateTimeFormat#exceptions.
If so, should we catch the error at all? It can come only from our invalid configuration, not from the user input.
ravicious
approved these changes
Mar 28, 2024
gzdunek
approved these changes
Mar 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a date time short formatter helper, that when in
testmode, useen-USas a default locale