-
Notifications
You must be signed in to change notification settings - Fork 4
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
Enable use of Intl.RTF #5
base: master
Are you sure you want to change the base?
Conversation
case "day": return this.rtf.format(diff.days, unit); | ||
case "hour": return this.rtf.format(diff.hours, unit); | ||
case "minute": return this.rtf.format(diff.minutes, unit); | ||
default: return this.rtf.format(diff.seconds, unit); |
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.
Cool to see Intl
support is indeed very straightforward.
Now, I would like to see if we can abstract the i18n library. Can we use format(rtf, value, unit)
where const format = RelativeTimeFormat.format
that wraps either Intl or globalize (in a plug-n-play way)? We should define how to plug that in. In my code below it was supposed to be overridden by any desired i18n lib, i.e., one should override RelativeTime.initializeFormatters
. Now, I assume we would replace that with two functions: RelativeTime.initializeFormatter
and RelativeTime.format
.
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'm open for different plug-n-play ideas too.
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 default wrappers could use Intl
.
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.
@rxaviers - Would you want to take it over? I'm not sure how you'd like to manage the pluggable backend
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.
Let's make relative-time v2 to support Intl
only? I can keep v1 to support globalize (and potentially a pluggable mechanism). Though, I believe the way to move forward is through standard Intl
constructor() { | ||
this.formatters = RelativeTime.initializeFormatters(...arguments); | ||
this.rtf = new Intl.RelativeTimeFormat(undefined); |
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.
We may need to include the numeric: "auto"
option here depending on how we solve tc39/proposal-intl-relative-time#60
I'd be great if we could get this going... :) |
@@ -39,9 +40,9 @@ function startOf(date, unit) { | |||
return date; | |||
} | |||
|
|||
export default class RelativeTime { |
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.
removing the export looks weird to me.
import Globalize from "globalize"; | ||
import ZonedDateTime from "zoned-date-time"; | ||
// import Globalize from "globalize"; | ||
// import ZonedDateTime from "zoned-date-time"; |
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.
There are still usages of ZonedDateTime in the code.
This is not ready for any reviews, but a good starting point for conversation.
This is all I had to do to make the code work with Intl.RTF. I'm not sure how much you want to allow for Globalize to be a backend and is there a way to write Intl.RTF polyfill that uses Globalize so that this file is just using Intl.RTF either platform impl or a polyfill.
Also, there seem to be an opportunity for more cleanups if we switch to RTF.