-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add new implementation of relative time #300
base: main
Are you sure you want to change the base?
Conversation
3cdcb9d
to
361ef2d
Compare
The failing test in the first check:
is caused by the original test using |
@@ -453,7 +454,7 @@ export class RelativeTimeElement extends HTMLElement implements Intl.DateTimeFor | |||
if (format === 'duration') { | |||
newText = this.#getDurationFormat(duration) | |||
} else if (format === 'relative') { | |||
newText = this.#getRelativeFormat(duration) | |||
newText = this.#getRelativeFormat(date) |
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.
The intent of using durations here is to anticipate Temporal
becoming part of the web platform.
We calculate the duration of the two dates and return a proxy object for Temporal.Duration
, so that we can in turn hope to call round
on this duration, to get a single unit duration object, which can then be more easily fed into Intl.RelativeTimeFormat
.
The Temporal.Duration#round
function is very complex, and so for now we have roundToSingleUnit
as a "simplified" version of this function, which provides the minimum viable requirements for this code.
The hope is that, once Temporal
is a part of the web platform, we can delete a lot of the delicate date code in this library.
The change in this PR, while reasonable in isolation, steps us further away from this goal of steering toward Temporal.
Having said that I'm sure this PR can be factored such that it maintains this goal. It might be worth looking at the round
API to figure out how to change the inputs to relativeTime
. Perhaps it is worth taking the functionality of relativeTime
and factoring it into roundToSingleUnit
?
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.
That makes me question then why an already existing implementation of the round
function could not be adopted.
Perhaps it is worth taking the functionality of
relativeTime
and factoring it intoroundToSingleUnit
?
Sure, it can be quickly done. Without knowing the Temporal
vision I made a new function with a different signature because calculating a duration then calculating back the original date would be a very roundabout thing to do. Perhaps then I will raise this concern to tc39/proposal-temporal
.
I have applied this implementation to |
(This is part of a set of multiple pull requests looking to overhaul the calculation functions.)
This pull request adds a new
relativeTime
function that directly takes two date values and outputs a single-unit relative time point, with more robust logic compared to the current implementation.Accordingly, the logic of the relative time element for the
relative
format is changed to use the new function.