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.

Sign up for GitHub

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

Web: intl add a date time short formatter #39915

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Web: intl add a date time short formatter #39915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7532a5
Web: intl add a date time short formatter
kimlisa Mar 27, 2024
d80c942
Address CRs
kimlisa Mar 28, 2024
9d1c5b7
Merge branch 'master' into lisa/date-time-short-format
kimlisa Mar 28, 2024
e53998f
Merge branch 'master' into lisa/date-time-short-format
kimlisa Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion web/packages/shared/services/loc/loc.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import { displayDate, displayDateTime } from './loc';
import { displayDate, displayDateTime, dateTimeShortFormat } from './loc';

const testDate = new Date('2022-01-28T16:00:44.309Z');

Expand All @@ -31,3 +31,7 @@ test('displayDateTime', () => {

expect(output).toBe('2022-01-28 16:00:44');
});

test('dateTimeShortFormat', () => {
expect(dateTimeShortFormat(testDate)).toEqual('4:00 PM');
});
16 changes: 16 additions & 0 deletions web/packages/shared/services/loc/loc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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 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.

try {
if (isTest) {
return new Intl.DateTimeFormat('en-US', { timeStyle: 'short' }).format(
date
);
}
return new Intl.DateTimeFormat(undefined, { timeStyle: 'short' }).format(
date
);
Copy link
Contributor

Choose a reason for hiding this comment

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

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';
Copy link
Member

@ravicious ravicious Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. What errors do we expect to be thrown here? I see that other functions in this file do use try … catch, but they also use format from date-fns and not Intl.DateTimeFormat.
  2. In case of an error, wouldn't it be better to use some kind of method on Date to display something approximate instead of saying "undefined"? If Intl.DateTimeFormat().format cannot fail, then the error handling is not even necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. From what I see, the function throws only when locales or options contain 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.

}
}
Loading