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.

Sign up for GitHub

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

Make strings in theme.json translatable #66675

Merged
merged 8 commits into from
Dec 23, 2024
Merged

Make strings in theme.json translatable #66675

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
42f4f5d
Allow theme.json's strings to be extracted by wp-cli
d-alleyne Oct 31, 2024
7505e51
Add shadow presets to theme-i18n.json to make them translatable
d-alleyne Nov 1, 2024
a03c3aa
Fix typo
d-alleyne Nov 1, 2024
6e9964b
Match the naming pattern
d-alleyne Dec 19, 2024
215a790
Revert "Allow theme.json's strings to be extracted by wp-cli"
d-alleyne Dec 19, 2024
8b1c87a
Implement code review suggestion
d-alleyne Dec 20, 2024
76ce9ee
Merge branch 'WordPress:trunk' into fix/translatable-strings-in-theme…
d-alleyne Dec 20, 2024
29e08a5
Add backlog file
d-alleyne Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/theme-i18n.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@
}
]
},
"shadow": {
"presets": [
{
"name": "Shadow presets name"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "Shadow presets name"
"name": "Shadow preset name"

Let's match the naming pattern

Copy link
Member

Choose a reason for hiding this comment

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

Can we perhaps use Shadow name instead? That's inline with what other presets do:

  • font sizes: "font size name"
  • colors: "color name"
  • aspect ratios: "aspect ratio name"

Copy link
Member

Choose a reason for hiding this comment

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

Also: these changes need to be backported to wordpress-develop so that the wp-cli uses them (see). Until then, these strings won't be picked up from anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oandregal , I created a diff for wordpress-develop, but it instructs that the description should have a trac number. Do we have one, or do I create one?

Copy link
Member

Choose a reason for hiding this comment

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

You can use https://meta.trac.wordpress.org/ticket/7866 I've left a comment linking to this PR and the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oandregal I had created https://core.trac.wordpress.org/ticket/62728. I hope it isn't an issue.

Copy link
Member

Choose a reason for hiding this comment

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

That's good as well 👍

}
]
},
"blocks": {
"*": {
"typography": {
Expand Down
1 change: 1 addition & 0 deletions theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
lib/theme.json
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually move the file instead of adding a symlink

Copy link
Member

Choose a reason for hiding this comment

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

The lib/theme.json location was working before; why change it now?

cc @oandregal

Copy link
Member

Choose a reason for hiding this comment

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

This is explained in #66027

WP-CLI assumes that theme.json only exists in a theme, and in themes it's required to be in the root. It does not look for any other subfolders and I don't wanna change that in WP-CLI.

The Gutenberg plugin is an exception because it holds the "source of truth" for theme.json and does so in the lib folder.

Copy link
Member

Choose a reason for hiding this comment

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

I need to catch up a bit on this, but last I checked (3 years ago) it was working fine (it extracted strings from core, Gutenberg, and themes).

Copy link
Member

Choose a reason for hiding this comment

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

Yes please test it again. But the way WP-CLI works hasn't changed: https://github.com/wp-cli/i18n-command/blob/0133eb1326e7e0613315dd94c347a2a03ee6f2d4/src/ThemeJsonExtractor.php#L19-L25

Copy link
Member

Choose a reason for hiding this comment

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

I've also searched for specific strings that are only defined in theme.json. Example: "Dark grayscale" for duotone. Same result, they are not present in translate:

Gutenberg WordPress
Screenshot 2024-12-18 at 17 43 23 Screenshot 2024-12-18 at 17 43 02

This comment was marked as outdated.

Sign in to view

Copy link
Member

Choose a reason for hiding this comment

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

More context: for testing, avoid using color gradients, palette, or font sizes. Some of these are also defined in the client (see) so it can give you false positives.

Copy link
Member

Choose a reason for hiding this comment

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

However, if I do the same search for WordPress core and Gutenberg, there are no strings coming from theme.json

With that current logic in WP-CLI that would be expected. As I said, it only looks for a specific location at the moment.

That change was made with the assumption that theme.json is supposed to live only in themes and in the root folder of theme. This helped avoid an issue where, when extracting strings in core, theme.json strings from all the default themes were erroneously included (because they live in wp-content.

I suppose we didn't realize at the time that wp-includes/theme.json also exists... 🤔

it sounds we haven't been extracting strings from the theme.json provided by WordPress (or Gutenberg) since 2 years ago?

Yes, you're right. Hmm we need to make some changes to WP-CLI then and to Meta (sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php)

Copy link
Member

Choose a reason for hiding this comment

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

Closing the loop here: we're continuing the conversation at wp-cli/i18n-command#423

Loading