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

theme.json: find title string within styles.blocks.variations #405

Merged
merged 5 commits into from
Jun 15, 2024

Conversation

oandregal
Copy link
Contributor

@oandregal oandregal commented Jun 13, 2024

Related PRs in this repo: #254 #292 #306
Related Gutenberg PR for this string: WordPress/gutenberg#62552

There's a new feature coming for WordPress 6.6 that allows theme authors to register block style variations through theme.json. Block style variations have a label that is displayed to users, so it needs to be translated. I just found out that there is a missing place where the label wasn't picked from. This PR follows suit on what is done in Gutenberg at WordPress/gutenberg#62552 There's an upcoming WordPress core PR as well.

Setup

brew install jq mysql
brew service start mysql
mysql_secure_installation
sudo mysql -u root -p

# MySQL Session
CREATE DATABASE IF NOT EXISTS `wp_cli_test`;
CREATE USER 'wp_cli_test'@'localhost' IDENTIFIED WITH mysql_native_password BY 'password1';
GRANT ALL PRIVILEGES ON wp_cli_test.* TO "wp_cli_test"@"localhost";
\q

Test

Before running any of the test (manual or automattic), make sure to set the THEME_JSON_SOURCE to empty so the ThemeJsonExtractor picks the the local backup file with the changes at assets/theme-i18n.json.

Automated tests:

composer install
composer behat -- features/makepot.feature

Manual testing:

  • Check out this PR locally and cd to the directory.
  • composer install
  • Create a new directory called foo-theme that contains a theme.json file with the following contents:
{
    "styles": {
      "blocks": {
        "variations": {
          "myVariation": {
            "title": "My variation",
            "blockTypes": [ "core/group" ],
            "color": {
              "background": "red"
            }
          }
        }
      }
    }
}
  • Create a new file called ember.json within foo-theme/styles.json with the following contents:
{
    "styles": {
      "blocks": {
        "variations": {
          "myVariation": {
            "title": "Other variation",
            "blockTypes": [ "core/group" ],
            "color": {
              "background": "red"
            }
          }
        }
      }
    }
}
  • Run vendor/bin/wp i18n make-pot foo-theme
  • Verify that there's a foo-theme/foo-theme.pot file and that the two variations labels are present as well as its context "Style variation name". You should see something like:
#: styles/ember.json
msgctxt "Style variation name"
msgid "Other variation"
msgstr ""

#: theme.json
msgctxt "Style variation name"
msgid "My variation"
msgstr ""

Comments

This also needs a WordPress core PR. WordPress/gutenberg#62552 will be backported with the necessary changes.

@oandregal
Copy link
Contributor Author

It's not ready yet, hence the draft status. Will finalize it in the following days.

@swissspidy
Copy link
Member

Why is this kind of stuff happening so late in the cycle? 🤔

FWIW we now have an automated GitHub Actions workflow updating the schema in this repo. It's based on the schema in WordPress core trunk.

@aaronrobertshaw
Copy link

This is the first I've setup a wp-cli command env locally so have limited context around how this should be done. Please take the following with a grain of salt 🙂

✅ Setting up the env went smoothly
✅ This PR manually tests as advertised following test instructions directly
✅ The generated pot file looks to contain the desired data

Screenshot 2024-06-14 at 11 27 45 AM

❓ The automated tests do fail locally for me as well

Screenshot 2024-06-14 at 11 32 30 AM

Why is this kind of stuff happening so late in the cycle? 🤔

This is probably due to an oversight of mine, sorry.

There was a bug fix required for the new section styles feature so that keys in the theme.json could reliably be presumed to be slugs and prevent an apparent duplication of the style within the UI.

@swissspidy
Copy link
Member

The automated tests do fail locally for me as well

It‘s because this change is not in core trunk yet.

@oandregal
Copy link
Contributor Author

oandregal commented Jun 14, 2024

It‘s because this change is not in core trunk yet.

How the behat tests find the schema? Don't they use the THEME_JSON_SOURCE? I've set that constant to empty (so, supposedly, it's using the local file with the updated property) but the test still fails 🤔

Am I running the right command?

composer behat -- features/makepot.feature
@swissspidy
Copy link
Member

Yes that's the command. Yes they use THEME_JSON_SOURCE (the remote file), falling back to the local file (THEME_JSON_FALLBACK).

That's why the tests are failing on CI.

Are you saying the tests are failing locally for you when trying to force using the fallback?

@oandregal
Copy link
Contributor Author

oandregal commented Jun 14, 2024

Are you saying the tests are failing locally for you when trying to force using the fallback?

Yeah, exactly 😕 I set that constant to empty and executed the local tests but they still fail:

Captura de ecrã 2024-06-14, às 08 47 26
@oandregal
Copy link
Contributor Author

Pushed a new test for files within styles/ folder as well 21ffdee

@oandregal oandregal marked this pull request as ready for review June 14, 2024 07:05
@oandregal oandregal requested a review from a team as a code owner June 14, 2024 07:05
@oandregal
Copy link
Contributor Author

Related WordPress PR with the schema changes at WordPress/wordpress-develop#6825 I'll merge it shortly, to unblock CI tests here.

@oandregal
Copy link
Contributor Author

Committed the schema changes at https://core.trac.wordpress.org/changeset/58412 I presume the CI test requires a re-triggering at some point for tests to pass.

@ernilambar
Copy link
Member

I have re-run the tests.

@swissspidy
Copy link
Member

@oandregal Looking at your core changeset, styles needs to be under settings. In your test it's not under settings. So the test needs to be fixed.

features/makepot.feature Outdated Show resolved Hide resolved
And the foo-theme/foo-theme.pot file should contain:
"""
msgctxt "Style variation name"
msgid "My style variation"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This output doesn't match the input (My variation). Fixed at 8465789

@oandregal
Copy link
Contributor Author

I've pushed a couple of fixes to the test: c918b7a and 8465789. It's now passing for me locally (both setting THEME_JSON_SOURCE to empty to use the local assets and leaving it as it is to use the remote one from core).

@swissspidy swissspidy merged commit 24caafc into wp-cli:main Jun 15, 2024
48 checks passed
@swissspidy swissspidy added this to the 2.6.2 milestone Jun 15, 2024
@oandregal oandregal deleted the add/theme-json-variation-title branch June 18, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants