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

Enable use of Intl.RTF #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enable use of Intl.RTF #5

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 12 additions & 34 deletions src/relative-time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Globalize from "globalize";
import ZonedDateTime from "zoned-date-time";
// import Globalize from "globalize";
// import ZonedDateTime from "zoned-date-time";
Copy link

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.


const second = 1e3;
const minute = 6e4;
Expand All @@ -22,7 +22,8 @@ function defineGetter(obj, prop, get) {
}

function startOf(date, unit) {
date = date instanceof ZonedDateTime ? date.clone() : new Date(date.getTime());
// date = date instanceof ZonedDateTime ? date.clone() : new Date(date.getTime());
date = new Date(date.getTime());
switch (unit) {
case "year": date.setMonth(0);
// falls through
Expand All @@ -39,9 +40,9 @@ function startOf(date, unit) {
return date;
}

export default class RelativeTime {
Copy link

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.

class RelativeTime {
constructor() {
this.formatters = RelativeTime.initializeFormatters(...arguments);
this.rtf = new Intl.RelativeTimeFormat(undefined);
Copy link
Owner

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

}

format(date, {timeZoneData = null, unit = "best-fit"} = {}) {
Expand Down Expand Up @@ -104,13 +105,13 @@ export default class RelativeTime {
}

switch(unit) {
case "year": return formatters.year(diff.years);
case "month": return formatters.month(diff.months);
case "year": return this.rtf.format(diff.years, unit);
case "month": return this.rtf.format(diff.months, unit);
// case "week": return formatters.week(diff.weeks);
case "day": return formatters.day(diff.days);
case "hour": return formatters.hour(diff.hours);
case "minute": return formatters.minute(diff.minutes);
default: return formatters.second(diff.seconds);
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);
Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Owner

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.

Copy link
Collaborator Author

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

Copy link
Owner

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

}
}
}
Expand All @@ -137,26 +138,3 @@ RelativeTime.threshold = {
minute: 59, // at least 59 minutes before using hour.
second: 59 // at least 59 seconds before using minute.
};

// TODO: Remove redundancy. The only reason this code is that ugly is to get
// supported by globalize-compiler (which reads the static formatters).
RelativeTime.initializeFormatters = function(globalize) {
if (globalize) {
return {
second: globalize.relativeTimeFormatter("second"),
minute: globalize.relativeTimeFormatter("minute"),
hour: globalize.relativeTimeFormatter("hour"),
day: globalize.relativeTimeFormatter("day"),
month: globalize.relativeTimeFormatter("month"),
year: globalize.relativeTimeFormatter("year")
};
}
return {
second: Globalize.relativeTimeFormatter("second"),
minute: Globalize.relativeTimeFormatter("minute"),
hour: Globalize.relativeTimeFormatter("hour"),
day: Globalize.relativeTimeFormatter("day"),
month: Globalize.relativeTimeFormatter("month"),
year: Globalize.relativeTimeFormatter("year")
};
};