Make WordPress Core

Opened 7 years ago

Closed 5 years ago

#39222 closed feature request (fixed)

Add JavaScript date helpers

Reported by: rmccue's profile rmccue Owned by: rmccue's profile rmccue
Milestone: 5.0 Priority: normal
Severity: normal Version: 4.7
Component: Date/Time Keywords: needs-docs has-unit-tests
Focuses: rest-api Cc:

Description

With the REST API, it's now possible to receive raw dates (ISO8601 format) on the frontend, without having the site's date preferences applied.

Formatting dates is a pain in JavaScript, and having it match the site preferences and translations is a pain too. We should introduce a helper library to simplify this, based on our PHP date_i18n() and human_time_diff() functions.

Attachments (3)

39222.diff (74.2 KB) - added by rmccue 7 years ago.
Add wp-date.js
39222.2.diff (76.3 KB) - added by rmccue 7 years ago.
Updated documentation, and other tweaks
39222.3.diff (81.6 KB) - added by timmydcrawford 7 years ago.

Download all attachments as: .zip

Change History (41)

#1 @rmccue
7 years ago

Note: for proper translations, we'd also need to include a solution for translations, as we need plurals. See #20491 for that. We shouldn't get stuck on that, but would be good to simultaneously solve that.

#2 @swissspidy
7 years ago

Related: #20491.

#3 @afercia
7 years ago

Related: #38342 and #38920 as the attempts to use the REST API there need some JS date formatting utility.

#4 follow-up: @nerrad
7 years ago

Would there be consideration given to using an existing js library for dealing with dates and times (formatting, localization etc). moment.js (https://momentjs.com) seems to be a well known library in use. I've used it in a number of projects and it makes datetimes in js less painful :)

Last edited 7 years ago by nerrad (previous) (diff)

#5 in reply to: ↑ 4 @rmccue
7 years ago

Replying to nerrad:

Would there be consideration given to using an existing js library for dealing with dates and times (formatting, localization etc). moment.js (https://momentjs.com) seems to be a well known library in use. I've used it in a number of projects and it makes datetimes in js less painful :)

Moment is pretty great, but the things we need aren't fully handled by it. Primarily, localisation is something we want to handle ourselves, as is date formatting (from settings).

#6 @nerrad
7 years ago

What is it about the moment localization (https://momentjs.com/docs/#/i18n/) that isn't sufficient? If you're talking about loading our own translations (that would be provided by the wp.i18n project) then you can provide moment custom translations sets.

Setting initial format on a moment object is simple enough as well.

It seems that extending a well known/widely used existing, compatible licensed javascript library that works well with dates and times would be preferable to re-inventing the wheel (again) here.

Last edited 7 years ago by nerrad (previous) (diff)

#7 @rmccue
7 years ago

It's more that we need to have a consistent translation platform across all of WordPress (see #20491). We can integrate the two, but we also need to ensure that the date formatting settings are also respected. In particular, the Date and Time formats from the General Settings page need to be handled, which requires mapping from PHP date codes. There are also a bunch of other pieces which change date formatting dependent on the locale (see date_i18n()).

While it is less work immediately to use Moment's locales, we would then have two translation tools to maintain, and which may not be in sync. This also increases the maintenance burden for the translation teams. It's worth the additional effort to have a consistent set of translations for maintainability.

That's not to say we shouldn't add Moment, but if we do, we need to be considered and purposeful in how we handle adding it, it's not just a drop-in.

This ticket was mentioned in Slack in #core-restapi by swissspidy. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by rmccue. View the logs.


7 years ago

#10 @rmccue
7 years ago

  • Owner set to rmccue
  • Status changed from new to assigned

I have a lovely little patch for this working. Ended up using moment, since it's very useful, and provides a good building block for other JS.

@rmccue
7 years ago

Add wp-date.js

#11 @rmccue
7 years ago

  • Keywords has-patch needs-unit-tests needs-docs added; needs-patch removed

39222.diff adds wp-date.js (and wp.date.*), which relies on moment.js (also included). It also removes the Ajax endpoints used for previewing formatted date/times, and replaces it with live JS instead:

http://i.imgur.com/Ht202ww.gif

This removes two admin-ajax.php endpoints (date_format and time_format), and moves them to wp-admin/includes/deprecated.php.

It introduces new functions you can use:

  • wp.date.format() - Format a date.
  • wp.date.date() - Format a date in the site's timezone.
  • wp.date.gmdate() - Format a date in UTC.
  • wp.date.date_i18n() - Format a date, and translate it.

There are some pieces missing from the date translations:

  • Ordinals aren't translated - this matches core, see #22225
  • There's no equivalent of human_time_diff() - This requires full i18n support (for plurals), and hence needs #20491 first.

Otherwise, should be basically complete feature-wise. This should probably have unit tests.

This needs docs once we merge it; I'm not sure if our JSDocs are parsed out anywhere right now, so in lieu of that, regular pages somewhere would be great.

This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.


7 years ago

#13 @rmccue
7 years ago

(If desired, we could actually have all the times on the page update live as well, such as the ones for the timezone selector. That requires a little extra work that I didn't feel was necessary, but if anyone thinks it's a good idea, go for it.)

This ticket was mentioned in Slack in #core-i18n by swissspidy. View the logs.


7 years ago

#15 follow-up: @flixos90
7 years ago

The patch is looking great as far as I can tell (but let's wait for further reviews by more JSy people than me). I had implemented the mapping in a similar way in another project (the implementation here is more complete though), so I guess it shows we're following an approach that makes sense. I agree that using Moment.js is the right choice as we don't need to reinvent the wheel, and mapping the PHP formats is rather straightforward.

Something minor to consider: Instead of hard-coding __( 'F j, Y g:i a' ) for the datetime setting, could we instead concatenate the date_format and time_format values with a space? Also, what is formats.timezone used for (I couldn't see it anywhere in the code, why is that necessary)?

#16 @nerrad
7 years ago

Looks great! Glad to see the decision to go with moment - it definitely removes much of the extra work that would have been done.

To echo @flixos90 and confirm the approach, I've done mapping of formats similarly in other projects as well.

Within the mapping, should the date argument be clarified in jsdocs so that its clear it is expecting a moment date object? maybe even rename the variable to dateMoment so its extra clear?

Last edited 7 years ago by nerrad (previous) (diff)

#17 follow-up: @swissspidy
7 years ago

could we instead concatenate the date_format and time_format values with a space?

Nope. String concatenation is always bad for i18n. Also, these strings already exists like that, see WP_Locale::_strings_for_pot().


Looking at wpDateSettings, it seems like a WP_Locale JS counterpart would be helpful. To get all these formats etc. of the current locale. Perhaps we can handle that in #20491 or a new ticket afterwards.

#18 in reply to: ↑ 17 @flixos90
7 years ago

Replying to swissspidy:

could we instead concatenate the date_format and time_format values with a space?

Nope. String concatenation is always bad for i18n. Also, these strings already exists like that, see WP_Locale::_strings_for_pot().

Ah right. What I don't like about it though is that there would be no way for the user to modify the datetime format. It might be worth considering an option for that. On the other hand this might look over-complicated to the user ("I already added date and time formats").
What about concatenating date and time format through another translation string sprintf( _x( '%1$s %2$s', 'concatenation of date and time format' ), $date_format, $time_format )? Then it would be manageable per locale. A locale that does not support concatenation of these values in any way could return an empty string, and in that case it would fall back to what we currently have there. We could even go as far as providing an option for users of a locale that doesn't support concatenation.

Yes, I might be overthinking this, but I think we need a bit more flexibility here.

#19 in reply to: ↑ 15 @rmccue
7 years ago

Replying to flixos90:

Something minor to consider: Instead of hard-coding __( 'F j, Y g:i a' ) for the datetime setting, could we instead concatenate the date_format and time_format values with a space?

This is one that's already hardcoded, IIRC, although looking at it now, appears the only place this is used is in the (deprecated) Links manager (links_updated_date_format option).

Would potentially make sense for this to be a wider setting in any case. There's some existing hardcoded date formats that could potentially be replaced with this (such as the date format in the posts list table).

FWIW, the comments list table currently concatenates date formats, and isn't configurable:

$submitted = sprintf( __( '%1$s at %2$s' ),
	/* translators: comment date format. See https://secure.php.net/date */
	get_comment_date( __( 'Y/m/d' ), $comment ),
	get_comment_date( __( 'g:i a' ), $comment )
);

Also, what is formats.timezone used for (I couldn't see it anywhere in the code, why is that necessary)?

It's used for the timezone selector's "Local time is ..." preview; I originally JS-ified that as well, but dropped it from the patch since it became a bit too complex. Can remove the format (the current one is translated at the top of options-general.php).

This ticket was mentioned in Slack in #core by rmccue. View the logs.


7 years ago

@rmccue
7 years ago

Updated documentation, and other tweaks

#21 @rmccue
7 years ago

39222.2.diff removes the unused timezone format, and updates the JSDoc significantly. (I've checked this locally using jsdoc, but needs a review to make sure it matches our standards if I missed something.) In the process, I noticed I accidentally had two definitions of Z, where one should have been z; fixed that up now.

This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by rmccue. View the logs.


7 years ago

#24 @timmydcrawford
7 years ago

Great to see moment.js chosen for the task here. Should we be adding in some test coverage for wp.date here as well? I'd be happy to help add in some tests.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


7 years ago

#26 follow-up: @aduth
7 years ago

A small naming point: shouldn't the function names be camel-cased, i.e. wp.date.dateI18n instead of wp.date.date_i18n? Or if too awkward, maybe a different name?

I see some value in aligning APIs between PHP and JavaScript, but not to the point of breaking language-specific conventions.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#naming-conventions

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#28 in reply to: ↑ 26 @rmccue
7 years ago

  • Keywords needs-patch added; has-patch removed

Replying to aduth:

A small naming point: shouldn't the function names be camel-cased, i.e. wp.date.dateI18n instead of wp.date.date_i18n? Or if too awkward, maybe a different name?

You're totally right, I totally forgot about that. :) This needs to be fixed up in the patch.

@timmydcrawford Tests would be fantastic. I have no experience with QUnit personally, so I didn't touch it here; if you want to help out there, please do!

#29 @timmydcrawford
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

The last diff didn't apply cleanly to trunk for me, but 39222.3.diff should apply cleanly now. I added @aduth 's suggested change to dateI18n naming and applied a few other formatting changes like declaring vars on the first line in closures, and not over-writing argument variables ( not sure if this is a standard, but one we followed in the Media Widgets ).

I also discovered a bug in format() logic where wp.date.formatMap methods were being called with the date argument - which could be a string, instead of the momentDate instance. And some qunit test coverage has been added.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#31 @desrosj
7 years ago

  • Keywords has-patch added; needs-patch removed

The most recent patch applied cleanly for me.

#32 @ocean90
7 years ago

  • Keywords needs-patch added; has-patch removed

39222.3.diff:

  • I can't select a default value anymore, whatever I click only Custom gets checked: https://cloudup.com/cVxrkkXR7kE
  • get_locale() should be get_user_locale() because $wp_locale is populated with the data of the user locale.
  • options.php is still using wp.date.date_i18n().
  • It looks like the strings for relativeTime in wp.date.setupLocale() aren't translatable.

#33 @youknowriad
7 years ago

Worth noting that we're feeling the need for these date handling utilities in the Gutenberg project. (see this WIP PR) https://github.com/WordPress/gutenberg/pull/841

  • Do you think we should make the localized moment instance available? const { moment } = wp.date;? moment have several utilities that could be helpful without using the higher level API proposed here.

#34 @ocean90
7 years ago

  • Milestone changed from 4.8 to Future Release

#35 @aduth
7 years ago

In addition to formatting, should we include date parsing utilities? As an example, in the Gutenberg project we've adapted the latest patch and have found we need to be able to determine whether a date received for a post from the REST API occurs in the future.

https://github.com/WordPress/gutenberg/blob/691ec08/editor/selectors.js#L73-L76

I can see a few ways we approach this:

  • Use GMT fields instead of site-localized, e.g. moment.utc( dateGmt ).isAfter( moment.utc() )
  • Configure moment to a default timezone offset with moment.tz (example)
  • Include in wp.date parsing utilities complementing formatters

The last option is most relevant to this ticket and has a few implications:

  • It would return a moment instance and therefore tie us more strongly to Moment
  • What would we name the function? dateParse for equivalence to PHP functions? Wondering whether we're doing ourselves any benefit by pretending to mimic PHP functions in JavaScript, vs. more natural naming like parse and format.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


7 years ago

#37 follow-up: @rmccue
5 years ago

Now that this is included with Gutenberg, can we close this ticket out?

#38 in reply to: ↑ 37 @netweb
5 years ago

  • Keywords needs-patch removed
  • Milestone changed from Future Release to 5.0
  • Resolution set to fixed
  • Status changed from assigned to closed

Replying to rmccue:

Now that this is included with Gutenberg, can we close this ticket out?

Sounds good, marking as fixed in 5.0

Note: See TracTickets for help on using tickets.