Skip to content

Conversation

@barthc
Copy link
Contributor

@barthc barthc commented Nov 15, 2024

Context

⛑️ https://secure.helpscout.net/conversation/2763010990/73925?folderId=8114575

Summary

Added i18n support for date format using the IntlDateFormatter This IntlDateFormatter uses the ISO date format codes instead of PHP date format.

@github-actions
Copy link

github-actions bot commented Nov 15, 2024

Warnings
⚠️ When ready, don't forget to request reviews on this pull request from your fellow wizards.

Generated by 🚫 dangerJS against e4d5747

Comment on lines 74 to 76
if ( $locale ) {
$formatter = datefmt_create(
$locale,
IntlDateFormatter::FULL,
IntlDateFormatter::FULL,
null,
null,
$modifier
);
$replace = $formatter->format( $timestamp );
} else {
// phpcs:ignore WordPress.DateTime.RestrictedFunctions.date_date
$replace = date( $modifier, $timestamp );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, if you use https://developer.wordpress.org/reference/functions/wp_date/, does that solve the issue?


new GW_Format_Date_Merge_Tag( array(
'form_id' => 123,
'locale' => 'fr_FR', // When this option is enabled, use the ISO date format codes instead. See https://framework.zend.com/manual/1.12/en/zend.date.constants.html#zend.date.constants.selfdefinedformats
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd comment this line out, as most people likely won't want French formatted dates 😃

@barthc barthc force-pushed the barth/add/73925-support-i18n branch from 4502624 to cce75d6 Compare November 19, 2024 08:11
Copy link
Contributor

@claygriffiths claygriffiths left a comment

Choose a reason for hiding this comment

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

One final bit of feedback, otherwise LGTM!

Comment on lines 74 to 76
if ( $locale ) {
switch_to_locale( $locale );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd put a switch back to the other locale right before the return of this function, just to be safe.

$current_locale = determine_locale();

Copy link
Contributor

@claygriffiths claygriffiths left a comment

Choose a reason for hiding this comment

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

Perfect! Nice work. S&M

@barthc barthc force-pushed the barth/add/73925-support-i18n branch from 3861381 to e4d5747 Compare November 19, 2024 18:50
@barthc barthc merged commit 5721c5a into master Nov 19, 2024
3 checks passed
@barthc barthc deleted the barth/add/73925-support-i18n branch November 19, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants