Use Intl.RelativeTimeFormat #104

josh · 2019-04-12T22:38:48Z

Aims to implement #98.

To-do

Feature detect Intl.RelativeTimeFormat.
Decide how to handle "micro" formatter.
Fix tests.
Implement locale attribute

src/relative-time.js

josh · 2019-04-12T22:47:17Z

doh, @koddsson did you already start working on this? (#103)

koddsson · 2019-04-15T07:52:27Z

@josh Nah I just started exploring and noticed that some tests for special cases that would break when we'd start using RelativeTimeFormat so I added those 😸

mislav · 2019-04-15T10:21:16Z

Is there going to be a locale attribute so that users may pass a locale specifier other than "en" down to Intl?
If there isn't a locale attribute, should we attempt to read the default locale from the document, e.g. from <html lang="...">? https://www.w3.org/International/questions/qa-html-language-declarations

keithamus · 2019-04-15T10:22:25Z

should we attempt to read the default locale from the document

navigator.language can be used

josh · 2019-05-13T21:48:44Z

Alright, got tests passing here. Could use some review please 😁

koddsson

This looks good to me.

dgraham

😎

test/relative-time.js

josh · 2019-05-15T21:15:43Z

Would also love some feedback on the effect of this change for people who can speak more than English 😁

dgraham · 2019-05-15T21:23:10Z

src/utils.js

+      try {
+        // eslint-disable-next-line flowtype/no-flow-fix-me-comments
+        // $FlowFixMe: missing RelativeTimeFormat type
+        format = new Intl.RelativeTimeFormat(undefined, options)


I think we want to pass en as the locale until we add a locale attribute to the custom element so it's configurable by the host app. That will prevent this text from appearing in the default locale for the browser with the rest of the text appearing in English.

Yeah, I'm down to support that, though I was just stealing from our existing makeFormatter.

https://github.com/github/time-elements/blob/4e6f95a34fc897626e794ed78f0f45701043de67/src/utils.js#L102

Will that need to change as well? Should I do this in a separate PR? Starting to wonder how this change is going to ship. It might be a major version bump. So could bundle this and that locale change in one release.

I wonder if the runtime's default locale is hardcoded to en in browsers and this is different from the user's preferred language setting in the operating system. Anyway, since undefined is already working for DateTimeFormat, we can use it here too.

A major version release would be appropriate since this changes the text output slightly.

We could couple that with <relative-time locale="…"> if it's not too much work. The only weird thing is that the locale attribute would only apply to browsers with RelativeTimeFormat support. The older code path would still use the hardcoded English text.

josh · 2019-05-15T21:29:40Z

Decide how to handle "micro" formatter.

Not sure what to do, or even if we have to deal with this.

muan

Would also love some feedback on the effect of this change for people who can speak more than English 😁

Can confirmed that relative timestamps within the month all look pretty reasonable. 🙆 Not sure if there's anything specific I should look for.

josh · 2019-05-20T16:51:51Z

Thanks! I'll move forward with merging this and open another PR attempt for the locale= attribute.

josh requested a review from a team April 12, 2019 22:38