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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add humanTimeDiff() date utility #4100

Closed
wants to merge 2 commits into from
Closed

Add humanTimeDiff() date utility #4100

wants to merge 2 commits into from

Conversation

jaswrks
Copy link
Contributor

@jaswrks jaswrks commented Dec 20, 2017

Description

The humanTimeDiff() utility is like human_time_diff() in PHP. However, this version also supports a third argument that enables an automatic suffix of 'ago' (past) or 'from now' (future) — taken from the current locale, via window._wpDateSettings.

Where is this used?

It currently is not used. I'm just submitting this for consideration, because I found this new utility to be helpful while working on Annotations in #4068

How Has This Been Tested?

Unit tests are included in this PR.

Types of changes

New date utility.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@jaswrks
Copy link
Contributor Author

jaswrks commented Dec 20, 2017

This is ready for review.

@adamsilverstein
Copy link
Member

@jaswrks since we already use moment.js in Gutenberg, could we use fromNow(); instead?

@jaswrks
Copy link
Contributor Author

jaswrks commented Dec 21, 2017

@adamsilverstein This PR uses the moment.js to() method, which is almost the same as from(). Is there a reason to use from() or fromNow() instead of to()? So far as I can tell, from() and to() both do the same thing, the only difference is which time you work from.

@adamsilverstein
Copy link
Member

@jaswrks ah, looks good! Sorry, I missed the use of moment.js.

@jaswrks
Copy link
Contributor Author

jaswrks commented Jan 16, 2018

Cool, thanks. Are you able to merge?

// Convert to moment.
const fromMoment = moment( from );
// Set the locale.
fromMoment.locale( window._wpDateSettings.l10n.locale );
Copy link
Contributor

Choose a reason for hiding this comment

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

Something that worries me here, is that I'm not sure these are translated properly because we do not use the built-in moment translations but we use generated translations from WP i18n files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it helps, the relevant translation strings for this Moment functionality can be found here in Gutenberg, which comes from this localization.

To your point, I think the remaining issue is that these strings have not yet been translated in Gutenberg. I'll see if I can resolve that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow - doesn't this line set the moment.locale and thus rely on moment.js translation functionality for the difference string? In my case for example _wpDateSettings.l10n.locale is en_US. https://momentjs.com/docs/#/i18n/

Copy link
Contributor Author

@jaswrks jaswrks Jan 16, 2018

Choose a reason for hiding this comment

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

Thanks for catching that! Yep, that line can be removed. Looks to me like that same line can be removed from the existing dateI18n() utility also. The locale is set at the end of this file already, in the proper way.

Copy link
Contributor Author

@jaswrks jaswrks Jan 16, 2018

Choose a reason for hiding this comment

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

Actually, no. Those lines can stay and are correct. The setupLocale() method is defining the translations for _wpDateSettings.l10n.locale, and then restoring the original default Moment locale. So it's still necessary to set the locale again before using Moment elsewhere in this file.

The translation strings are already prepared for this, given the setupLocale() function that runs in this file. So the following:

fromMoment.locale( window._wpDateSettings.l10n.locale )

Is setting the locale (e.g., en_US), which has had its translation strings preconfigured ahead of time by setupLocale() in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To understand this better, see: https://momentjs.com/docs/#/customization/
And then look at what setupLocale() does in this Gutenberg file, and where setupLocale() is called from when the file is imported.

@jaswrks
Copy link
Contributor Author

jaswrks commented Jan 16, 2018

Appreciate the help :-) In c01d7b1 I translated a few remaining strings that are needed by relative time calculations in Moment, which were previously hard-coded in Gutenberg. They now come from _wpDateSettings. Also added a missing ss relativeTime string that did not exist prior.

Jason Caldwell added 2 commits January 17, 2018 12:59
The `humanTimeDiff()` utility is like `human_time_diff()` in PHP.
However, this version also supports a third argument that enables
an automatic suffix of `ago` (past) or `from now` (future).
@gziolo
Copy link
Member

gziolo commented Jun 10, 2018

There was no activity in this PR for almost 6 months. It looks like it isn’t required feature for Gutenberg core. Let’s close it. We can always reopen and rebase it with master branch to align with the current state of Gutenberg.

@gziolo gziolo closed this Jun 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants