Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

WIP: Add translation functions to text strings #489

Merged
merged 36 commits into from
Oct 7, 2024

Conversation

carolinan
Copy link
Contributor

@carolinan carolinan commented Oct 2, 2024

Description
This PR adds translation functions around text strings in patterns.

I have;

  • Ran the npm script to add the translation functions.
  • Adjusted some strings that the script broke.
  • Removed some trailing, duplicate periods in the "no results" texts.
  • Updated some strings to use printf.
  • Added context to some strings.
  • Ran the PHP lint script

To do:

  • Replace the curly single quotes etc with their HTML entities.
  • Adjust the split text "Starting at $30 /month" in the "Call to action with grid layout with products and link" pattern
  • Make the post term separators translatable <!-- wp:post-terms {"term":"category","separator":", ","style":{"typography":{"textTransform":"uppercase","letterSpacing":"1.4px"}}} /-->
  • One of these blocks are in an HTML file and needs to either be reset to the default value, or be moved to a pattern.

Testing Instructions
Review the code changes to find mistakes and things that can be improved further. ❤️
Preview and place templates and patterns to make sure there are no block validation errors.

@carolinan carolinan changed the title WIAdd translation functions to text strings WIP: Add translation functions to text strings Oct 2, 2024
Copy link

github-actions bot commented Oct 2, 2024

Preview changes

You can preview these changes by following the link below:

I will update this comment with the latest preview links as you push more changes to this PR.
⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

Copy link

github-actions bot commented Oct 2, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: carolinan <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: beafialho <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@carolinan carolinan added [Priority] High Used to indicate top priority items that need quick attention Internationalization (i18n) Issues or PRs related to internationalization efforts labels Oct 2, 2024
@carolinan carolinan added the [Status] In Progress Tracking issues with work in progress label Oct 2, 2024
@carolinan carolinan requested a review from t-hamano October 2, 2024 11:45
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thank you for all the hard work you've put in. I'll leave some feedback, even though it's all small things.

patterns/cta-book-links.php Outdated Show resolved Hide resolved
patterns/cta-events-list.php Outdated Show resolved Hide resolved
patterns/media-instagram-grid.php Outdated Show resolved Hide resolved
patterns/overlapped-images.php Outdated Show resolved Hide resolved
patterns/page-portfolio-home.php Outdated Show resolved Hide resolved
@t-hamano
Copy link
Contributor

t-hamano commented Oct 2, 2024

P.S. I don't think this is required, but if there is only one placeholder in the text, it might be better to use $s instead of %1$s.

Example:

/* Translators: Designed with WordPress. %1$s: WordPress link. */
esc_html__( 'Designed with %1$s', 'twentytwentyfive' ),

@carolinan
Copy link
Contributor Author

carolinan commented Oct 3, 2024

P.S. I don't think this is required, but if there is only one placeholder in the text, it might be better to use $s instead of %1$s.

Example:

/* Translators: Designed with WordPress. %1$s: WordPress link. */
esc_html__( 'Designed with %1$s', 'twentytwentyfive' ),

Looks like the handbook recommends using %s for single string placeholders. I will change it to that.
https://developer.wordpress.org/apis/internationalization/internationalization-guidelines/#variables

patterns/cta-book-links.php Outdated Show resolved Hide resolved
@carolinan
Copy link
Contributor Author

@beafialho Can you help clarify the separator in the post terms block? Because the default is a comma and a space, but in the theme it is set to a comma and two spaces. Because it is not the default value, it needs to be translation ready, which creates extra work including moving one block to a new pattern.

@t-hamano t-hamano self-requested a review October 4, 2024 06:44
@beafialho
Copy link
Contributor

@beafialho Can you help clarify the separator in the post terms block? Because the default is a comma and a space, but in the theme it is set to a comma and two spaces. Because it is not the default value, it needs to be translation ready, which creates extra work including moving one block to a new pattern.

Apologies but I'm not understanding clearly what it is you're asking.

@carolinan
Copy link
Contributor Author

carolinan commented Oct 4, 2024

@beafialho I will try again :)
The post terms block lists the categories and tags.
By default, the separator between these items are a comma followed by a space: ,
But in the theme, this has been updated to use two spaces.
Can this extra space be removed?

If not, there is a part of the default single post template, that needs to be created as a pattern, only to add this second space, which feels wasteful and not performant.

@beafialho
Copy link
Contributor

Thanks, I got what you meant now :)

I am seeing the separator as a comma followed by a space when I add post terms though.

@carolinan
Copy link
Contributor Author

Thanks, I got what you meant now :)

I am seeing the separator as a comma followed by a space when I add post terms though.

Yes, when we insert a new copy of the block it uses the default. It is in some of the patterns and in the default single post that it is using two spaces.

@beafialho
Copy link
Contributor

Yes, when we insert a new copy of the block it uses the default. It is in some of the patterns and in the default single post that it is using two spaces.

I'm sorry if it's obvious or written somewhere, but could you tell me which ones use the two spaces?

@carolinan
Copy link
Contributor Author

I have replaced the example dates with a single date, because the number of unique text strings to translate is very large, 266.
And that is not including the pattern and template names, color palettes, typography presets etc that will add more than 100 strings.

@carolinan
Copy link
Contributor Author

Stupid question, what is "Now" referring to? It needs some context because the translation can be different.
image

@carolinan
Copy link
Contributor Author

carolinan commented Oct 5, 2024

I have looked closer at the terms block with the custom separator, and I was wrong.
I found that all custom separators except for one, uses the pill style.
The space characters are used to create the spacing between the pills and to make sure that the default comma is not visible.
Because of that, my suggestion would be that this does not need to be translation ready, and we would not make any changes.


The remaining custom separator uses the comma followed by two spaces for the categories in "News blog single post with sidebar".
Screenshot with a comma and two spaces:
image
Screenshot with a comma and one space (default):
image

So I think the custom separator was perhaps a mistake.

@t-hamano
Copy link
Contributor

t-hamano commented Oct 5, 2024

Stupid question, what is "Now" referring to? It needs some context because the translation can be different.

The term "Now" was already present when the pattern was first created, and it appears to be so in the Figma file: #174

I don't know what it means too...

my suggestion would be that this does not need to be translation ready, and we would not make any changes.

I agree with this. It's not translated in the Twenty Tweny-Four theme either:

https://github.com/WordPress/wordpress-develop/blob/6c1d60453ab0b4d689f94673f8e6b530ff4b7cbc/src/wp-content/themes/twentytwentyfour/templates/single-with-sidebar.html#L28

@carolinan
Copy link
Contributor Author

I am interpreting "Now" as a "'Link to a page with information about what the person is working on right now."

@carolinan
Copy link
Contributor Author

I am going to merge this to unblock some other pattern changes, because I want to avoid merge conflicts.

@carolinan carolinan merged commit 054494b into trunk Oct 7, 2024
4 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants