-
Notifications
You must be signed in to change notification settings - Fork 187
Use today for today, and omit current year #338
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
base: main
Are you sure you want to change the base?
Conversation
zaataylor
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
| #isToday(date: Date): boolean { | ||
| const now = new Date() | ||
| const formatter = new Intl.DateTimeFormat(this.#lang, { | ||
| timeZone: this.timeZone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, having this be sensitive to user's TZ will save us many-a bug report!
| } | ||
|
|
||
| if ((format === 'relative' || format === 'duration') && !displayUserPreferredAbsoluteTime) { | ||
| const shouldObserve = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for breaking out that long conditional, this is much more readable to me
| test('respects locale formatting', async () => { | ||
| freezeTime(new Date('2023-01-15T17:00:00.000Z')) | ||
|
|
||
| document.documentElement.setAttribute('data-prefers-absolute-time', 'true') | ||
| const el = document.createElement('relative-time') | ||
| el.setAttribute('lang', 'es-ES') | ||
| el.setAttribute('time-zone', 'Europe/Madrid') | ||
|
|
||
| el.setAttribute('datetime', '2023-01-15T17:00:00.000Z') | ||
| await Promise.resolve() | ||
|
|
||
| // Spanish formatting - "hoy" = "today", 24-hour format | ||
| assert.equal(el.shadowRoot.textContent, 'Hoy 18:00 CET') | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the future! l10n!
| }) | ||
|
|
||
| suite('experimental: [data-prefers-absolute-time]', async () => { | ||
| teardown(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had initially wondered if we'd need a test for the boundary condition of moving from Today to the full timestamp, but this should "just work" given how we have the observable behavior set up!
Based on feedback, this PR updates the format when users prefer absolute time, to provide more contextual information for recent dates:
This was a decision based upon user feedback.
Approach
The desired format
Today, 2:34 PM EDTisn't natively supported byIntl.DateTimeFormat, so this PR makes a best attempt at it by combining:RelativeTimeFormat)DateTimeFormat)Assumptions & trade-offs
This approach assumes:
Alternative considered
Hard-coding this custom format for English-only, but feels unideal/inconsistent to special-case a format for english-only.
Examples
English:
TBD