Make WordPress Core

Opened 7 years ago

Closed 2 years ago

Last modified 2 years ago

#39733 closed enhancement (fixed)

List item separator should be a WP_Locale property

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: audrasjb's profile audrasjb
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: I18N Keywords: good-first-bug has-patch commit has-screenshots needs-dev-note
Focuses: Cc:

Description

Currently, some (most?) themes have a translatable list item separator for displaying a list of categories or tags. See an example in Twenty Seventeen:

/* translators: used between list items, there is a space after the comma */
$separate_meta = __( ', ', 'twentyseventeen' );

One of ru_RU translation team contributors made a point that list item separator is a locale property, and it doesn't make much sense to translate it separately in multiple projects.

We could probably add it as a property of WP_Locale class, in addition to ::number_format['thousands_sep'] and ::number_format['decimal_point'], and there should be a public function like number_format_i18n(), e.g. get_list_item_separator(), for plugins and themes to use. Thoughts?

Attachments (5)

39733.2021091800.patch (19.9 KB) - added by rsiddharth 2 years ago.
Initial version (2021091800)
39733.2021092000.patch (16.0 KB) - added by rsiddharth 2 years ago.
Version 2021092000
39733.2022011300.patch (19.2 KB) - added by rsiddharth 2 years ago.
Capture d’écran 2022-03-13 à 19.58.38.png (176.0 KB) - added by audrasjb 2 years ago.
After patch: separator works on WP-Admin
Capture d’écran 2022-03-13 à 19.58.57.png (258.2 KB) - added by audrasjb 2 years ago.
After patch: separator works on front-end (example: Twenty Thirteen)

Download all attachments as: .zip

Change History (21)

#1 @swissspidy
7 years ago

Sounds useful. Besides the default themes there are three instances of __( ', ' ) being used in core. Perhaps more when counting instances of implode( ', ', … ) that could make use of a translatable list item separator.

#2 @swissspidy
3 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

@rsiddharth
2 years ago

Initial version (2021091800)

#3 @rsiddharth
2 years ago

  • Keywords has-patch added; needs-patch removed

@swissspidy @SergeyBiryukov I've uploaded the initial version of the patch.

Are you able to tell if I'm going in the correct direction?

#4 @johnbillion
2 years ago

Just noting that I don't think it will be possible to change the default themes to use this new method because they need to remain compatible with the version of WordPress where they were introduced. A compatibility layer in each theme could be an option.

#5 @SergeyBiryukov
2 years ago

Thanks for the patch! The WP_Locale part looks good at a glance.

For the usage though, I think we'd want to avoid using the $wp_locale global in all those instances, and just wrap this in a function like wp_get_list_item_separator() that would still call $wp_locale->get_list_item_separator() internally.

As noted above, we should also think about backward compatibility for bundled themes.

@rsiddharth
2 years ago

Version 2021092000

#6 @rsiddharth
2 years ago

@johnbillion @SergeyBiryukov I uploaded the second version of the patch.

Changes:

  • Added wrapper function -- wp_get_list_item_separator() -- for WP_Locale::get_list_item_separator.
  • Replaced $wp_locale->get_list_item_separator() calls with wp_get_list_item_separator() calls.
  • Added a "compatibility layer" for the twentyeleven theme. Does that look correct?
Last edited 2 years ago by rsiddharth (previous) (diff)

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


2 years ago

#8 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 6.0

#10 @audrasjb
2 years ago

The above PR refreshes the previous patch against trunk and updates @since mentions. It also add again a removed translator comment.

#11 @audrasjb
2 years ago

  • Owner set to audrasjb
  • Status changed from new to accepted

The patch works fine on my side using default themes or navigating through admin screens.
Self assigning for commit consideration.

@audrasjb
2 years ago

After patch: separator works on WP-Admin

@audrasjb
2 years ago

After patch: separator works on front-end (example: Twenty Thirteen)

#12 @audrasjb
2 years ago

  • Keywords commit has-screenshots added

Manual tests are going well. Marking for commit.

#13 @audrasjb
2 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 52929:

i18n: Define List item separator as a WP_Locale property.

The list item separator is a locale property, and it doesn't make much sense to translate it separately in multiple projects. This changeset implements the following modifications:

  • Define list item separator as a new WP_Locale property
  • Add wp_get_list_item_separator() as a wrapper for WP_Locale::get_list_item_separator
  • Replace $wp_locale->get_list_item_separator() calls with wp_get_list_item_separator()
  • Added a compatibility layer for bundled themes

Props SergeyBiryukov, swissspidy, rsiddharth, johnbillion, audrasjb.
Fixes #39733.

#14 @audrasjb
2 years ago

  • Keywords needs-dev-note added

This change deserves a dev note or at least a mention in the Misc changes dev note.

#16 @SergeyBiryukov
2 years ago

In 52933:

I18N: Move wp_get_list_item_separator() to a more appropriate place.

Follow-up to [52929].

See #39733.

Note: See TracTickets for help on using tickets.