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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃彈 amp-timeago:1.0 - Use timeago.js library #32844

Merged
merged 5 commits into from Feb 24, 2021

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Feb 23, 2021

This PR:

  • Migrates amp-timeago 1.0 from using the timeago package in third_party/ to the runtime dependency package
  • Upgrades the version of timeago.js being used by amp-timeago:1.0 from 3.0.0 to 4.0.2
  • Removes third_party/timeagojs as it's no longer needed given the above.
  • Updates both versions of amp-timeago and corresponding docs to use locale values of the format xx or xx-XX (case-insensitive) in accordance with BCP 47. Note: Prior formats (xxXX) are still supported but removed from docs to move away from non-standard formatting, and timeago.js library formats (xx_XX) were not added to the larger set of supported formats.

This PR is a follow up to #32809. Related to #29246.

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! So much cleaner, with no real difference to binary size.

image

Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT. But not seeing timeago package.json changes. Was that done already before?

@@ -60,6 +60,11 @@ export function Timeago({
return undefined;
}
const observer = new win.IntersectionObserver((entries) => {
let {lang} = node.ownerDocument.documentElement;
if (lang === 'unknown') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never seen "unknown", but I do see an empty string. There's also navigator.language that could be helpful for defaults.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got unknown from this reference: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/lang, should I remove this check?

re: package.json changes, this was done in #32809 for 0.1 separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to merge this PR for now, let's follow up separately if we decide to remove "unknown"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants