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

Move Global Styles code to lib/compat/wordpress-5.9 folder #36978

Merged
merged 1 commit into from
Dec 1, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Nov 29, 2021

Related #36957 (comment)
Depends on #36974

This PR moves the global styles classes code under lib/compat/wordpress-5.9 folder. The rationale for this is to make sure we have a path to remove this code from the plugin.

Files moved to the 5.9 directory:

  • lib/class-wp-theme-json-gutenberg.php
  • lib/class-wp-theme-json-resolver-gutenberg.php
  • lib/class-wp-theme-json-schema-gutenberg.php
  • lib/theme.json
  • lib/theme-i18n.json

How to test

  • Verify that automated tests pass.
  • Use a theme you're familiar with (I did it with TT1-blocks) and verify that styles look as expected in the post editor, site editor, and front end.
  • Also check that the presets (colors, font sizes) are present in the block inspector and global styles sidebar.

@oandregal oandregal self-assigned this Nov 29, 2021
@oandregal oandregal added [Type] Code Quality Issues or PRs that relate to code quality Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Nov 29, 2021
@oandregal oandregal changed the base branch from trunk to update/theme-json-resolver-class November 29, 2021 22:00
Base automatically changed from update/theme-json-resolver-class to trunk November 29, 2021 22:18
@oandregal oandregal force-pushed the update/move-gs-classes-to-compat-folder branch from ed7ab59 to cf351bb Compare November 29, 2021 22:19
@oandregal
Copy link
Member Author

I've tested that moving the theme.json file within this directory is fine in terms of translations. This is how I've tested:

  • cloned the repo of the i18n command used by the cli to extract strings https://github.com/wp-cli/i18n-command/
  • cd i18n-command && composer install
  • vendor/bin/wp i18n make-pot <path/to/gutenberg/repo/using/this/branch>

The expected result is that there's a gutenberg.pot file in the top level directory of Gutenberg. I inspected its contents and verified that strings in lib/compat/wordpress-5.9/theme.json are extracted by looking for references of that file within the pot:

#: lib/compat/wordpress-5.9/theme.json
msgctxt "Font size name"
msgid "Small"
msgstr ""

#: lib/compat/wordpress-5.9/theme.json
msgctxt "Font size name"
msgid "Normal"
msgstr ""

# etc.

@oandregal oandregal marked this pull request as ready for review November 30, 2021 12:07
@oandregal oandregal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Nov 30, 2021
@oandregal
Copy link
Member Author

Marked as to backport to make sure this lands in Gutenberg's wp/trunk branch. No need to do anything for WordPress core.

@ntsekouras
Copy link
Contributor

Do we need to move theme.json as well? Don't we use the version of theme.json to indicate changes? Is it about the default values there?

@oandregal
Copy link
Member Author

oandregal commented Nov 30, 2021

Yeah, we should. theme.json is just another part of the configuration and is related to the code that processes it. If the change is trivial (true => false) we may not need code changes but still need to track which WordPress version it was part of. For other kinds of changes (like adding a new key, etc) we do need it to be in sync with the code that knows how to interpret that info.

@aaronrobertshaw aaronrobertshaw self-requested a review December 1, 2021 01:13
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This tests well for me 👍

✅ Automated tests pass
✅ Styles appeared as per normal in post editor, site editor and frontend
(tested with TT1 Blocks & TwentyTwentyTwo)
✅ Presets, such as colors and font sizes, were present in both block inspector and global styles sidebar
✅ I18n ran successfully, relocated theme.json entries were present and matching in generated .pot file

There was one failing test when running npm run test-php though that was fixed in #36987 yesterday.

The reasoning for moving theme.json as well makes sense to me also.

@youknowriad
Copy link
Contributor

youknowriad commented Dec 1, 2021

What happens now with WordPress trunk? Do we still load these classes instead of Core, should we guard against that (to avoid having folks changing these files inadvertently and forgetting the backport)

@oandregal
Copy link
Member Author

What happens now with WordPress trunk? Do we still load these classes instead of Core

Yeah, we use these classes instead of WordPress trunk for some checks. Specifically for:

  • has_theme_support
  • get_custom_templates
  • get_template_parts
  • and some places that don't use yet the public API to retrieve settings/styles (rest API)

I think a goal for 6.0 should be to 1) create the public methods for the things that don't have them yet and 2) use those methods everywhere so the classes aren't used directly. For most of the code should be straightforward and I can start doing that already.

should we guard against that (to avoid having folks changing these files inadvertently and forgetting the backport)

I'm hoping that changing code that lives in lib/compat/ is a strong enough signal that you shouldn't change it, no matter whether its name has a gutenberg affix in it. I'm on board to create a safeguard, though. That'd be ideal. I'll look into that. It's not going to be a straightforward class_exist check as the class already exists in WordPress 5.8, in which case we still want to override them.

Do you think having this safeguard is a blocker for this merging this PR?

I ask because I see that there's more code that does the same (the plugin uses the version from lib/compat instead of the backported one).

@youknowriad
Copy link
Contributor

Do you think having this safeguard is a blocker for this merging this PR?

No, I don't think it's a blocker but it shouldn't be too complex to add. @talldan is doing something similar in https://github.com/WordPress/gutenberg/pull/36898/files#diff-e0498912315fab82090a3c4953f6f616455946b41a885dd0a131e22a261d0475R16

@oandregal oandregal merged commit 61de106 into trunk Dec 1, 2021
@oandregal oandregal deleted the update/move-gs-classes-to-compat-folder branch December 1, 2021 10:41
@github-actions github-actions bot added this to the Gutenberg 12.1 milestone Dec 1, 2021
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Dec 6, 2021
@oandregal
Copy link
Member Author

Follow up for global styles CPT registration at #37282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants