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

Time To Read Block: Fix untranslated on front end #49704

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Apr 11, 2023

Fixes: #49694

What?

This PR fixes a problem where the unit of time is not translated when the Post Time to Read block is rendered on the front end.

Why?

I have changed the placeholder in the _n() function string from %d to %s and have confirmed that it translates correctly. I don't know the root cause, but in the WordPress core, %s is used as a placeholder in the _n() function as well. Also in Developer Resources, all examples use %s as a placeholder.

Testing Instructions

  • Create two posts, one with long content and one with short content.
  • Open the site editor.
  • Open Home template.
  • Insert a Post Time to Read block inside a Query Loop block.
  • Change the language of the site.
  • Confirm that both singular and plural are translated correctly in the front end.

Screenshots or screencast

Japanese

japanese

German

german

Spanish

spanish

@t-hamano t-hamano self-assigned this Apr 11, 2023
@t-hamano t-hamano added Internationalization (i18n) Issues or PRs related to internationalization efforts [Block] Time to Read Affects the Time to Read Block labels Apr 11, 2023
@github-actions
Copy link

Flaky tests detected in dd22814.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4662975713
📝 Reported issues:

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

I didn't dig deep to see why we need to have %s, but in every similar usage in core is like this. Example:

/* translators: %s: Number of comments. */
$new_text = _n( '%s Comment', '%s Comments', $number );

I think we can land this. Although it would be cool to pinpoint the place in code that this is required to work properly.

@t-hamano t-hamano marked this pull request as ready for review April 11, 2023 12:06
@t-hamano t-hamano requested a review from ajitbohra as a code owner April 11, 2023 12:06
@t-hamano
Copy link
Contributor Author

@ntsekouras

Thank you for the review!

I have not been able to find the root cause, but looking at the core codebase and Developer Resources, it appears that the _n() function is supposed to use %s as a placeholder. It's a little unclear, but I'll merge it.

@t-hamano t-hamano merged commit f587246 into trunk Apr 11, 2023
@t-hamano t-hamano deleted the fix/time-to-read-translate branch April 11, 2023 12:11
@github-actions github-actions bot added this to the Gutenberg 15.6 milestone Apr 11, 2023
@bph bph added the [Type] Bug An existing feature does not function as intended label Apr 11, 2023
@swissspidy
Copy link
Member

I have not been able to find the root cause, but looking at the core codebase and Developer Resources, it appears that the _n() function is supposed to use %s as a placeholder. It's a little unclear, but I'll merge it.

That's not really the root cause. The reason is that the translation call here uses the WordPress default text domain ('default') instead of 'gutenberg' (a special case for the Gutenberg project). That means even though the string %d minute was translated on translate.wordpress.org, it didn't matter because it's the wrong text domain.

Coincidentally, the strings %s minute and %s minutes already exist in core (in human_readable_duration()) with the default text domain and thus are already translated for core. Since you changed the strings here to match core, it magically worked.

Hope that helps.

@swissspidy
Copy link
Member

Oh, and WordPress often uses %s for numeric placeholders because the numbers are typically formatted (e.g. with decimal or thousands separator)

@t-hamano
Copy link
Contributor Author

Thank you for the information, @swissspidy.

Am I correct in understanding that after a major release of WordPress core (i.e., after Gutenberg is bundled with core), any new translations added on the Gutenberg plugin will not be translated unless that translation exists in WordPress core?

If so, then maybe this PR was pointless 😅

@swissspidy
Copy link
Member

It seems like that is the case, yes. But haven't checked if there's something special going on elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Time to Read Affects the Time to Read Block Internationalization (i18n) Issues or PRs related to internationalization efforts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Time To Read Block - Translations are ignored
4 participants