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

Feature: Block-based template parts for Classic themes #3234

Conversation

Mamaduka
Copy link
Member

PR backports changes are required for using block-based template parts in classic themes - WordPress/gutenberg#42729.

Gutenberg tracking issue: WordPress/gutenberg#43440
Trac ticket: https://core.trac.wordpress.org/ticket/56467

Testing Instructions

Classic theme - TT1

  • The new "Template Parts" menu shouldn't appear in Appearance.
  • Users can't access the /wp-admin/site-editor.php path directly.

Block theme - TT2

  • The new "Template Parts" menu shouldn't appear in Appearance.
  • The "Editor" menu is accessible.
  • The Site Editor works as before.

Themes with block-based templates part enabled
You can grab the testing theme from this repo - https://github.com/Mamaduka/block-fragments

  • The new "Template Parts" menu is visible under Appearance.
  • The menu redirects to the "Template Parts" list view.
  • The "Site" and "Templates" navigation items aren't visible.

Warning
The last item will require the latest package update PR to be merged, and it might not be possible to test immediately.


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

'styles' => get_block_editor_theme_styles(),
'defaultTemplateTypes' => $indexed_template_types,
'defaultTemplatePartAreas' => get_allowed_block_template_part_areas(),
'supportsLayout' => WP_Theme_JSON_Resolver::theme_has_support(),
Copy link
Member Author

Choose a reason for hiding this comment

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

Before, the Site Editor always assumed layout support.

@@ -5261,7 +5261,7 @@ function wp_widgets_add_menu() {
}

$menu_name = __( 'Widgets' );
if ( wp_is_block_theme() ) {
if ( wp_is_block_theme() || current_theme_supports( 'block-template-parts' ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We need this to avoid conflicts between the Customize and Widgets menu positions.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Mamaduka! LGTM :shipit:

@hellofromtonya hellofromtonya self-assigned this Sep 14, 2022
@hellofromtonya
Copy link
Contributor

@ockham what is the status of the Warning in the description?

The last item will require the latest package update PR to be merged, and it might not be possible to test immediately.

@ockham
Copy link
Contributor

ockham commented Sep 14, 2022

@ockham what is the status of the Warning in the description?

The last item will require the latest package update PR to be merged, and it might not be possible to test immediately.

Ah, my bad, I should've elaborated on that. I was indeed able to test and confirm everything except for

The "Site" and "Templates" navigation items aren't visible.

As @Mamaduka said, we'll only be able to verify this once we've synced @wordpress/ packages. That sync is still blocked. However, I don't think we need to hold off until that package sync, since part of the logic responsible for conditionally showing the "Site" and "Templates" menu items lives in JS (and can be considered part of those packages, rather than the PHP that this PR touches).

In any case, we can easily test this separately once we sync packages.

Does that sound okay?

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Sep 14, 2022

Testing Report

Env:

  • WordPress: trunk
  • Plugins: none
  • Browser: Firefox
  • Localhost: wp-env

Results:

Classic theme - TT1

  • The new "Template Parts" menu shouldn't appear in Appearance ✅
  • Users can't access the /wp-admin/site-editor.php path directly ✅

Screen Shot 2022-09-14 at 11 55 15 AM

Block theme - TT2

  • The new "Template Parts" menu shouldn't appear in Appearance ✅
  • The "Editor" menu is accessible ✅
  • The Site Editor works as before ✅

Themes with block-based templates part enabled
Theme: https://github.com/Mamaduka/block-fragments

  • The new "Template Parts" menu is visible under Appearance ✅
  • The menu redirects to the "Template Parts" list view ✅
  • The "Site" and "Templates" navigation items aren't visible ❌ (needs the JS packages updated - per notes in the description)

Screen Shot 2022-09-14 at 11 52 36 AM

Observations:

  • With the block-fragments (classic theme with block-based templates) enabled, I am able to directly access /wp-admin/site-editor.php but it's a white screen. No errors in the browser's console or error logs.

Screen Shot 2022-09-14 at 11 53 34 AM

@Mamaduka @ockham Is this something that will be fixed with another backport? Or a known issue?

@@ -19,7 +19,7 @@
);
}

if ( ! wp_is_block_theme() ) {
if ( ! ( current_theme_supports( 'block-template-parts' ) || wp_is_block_theme() ) ) {
wp_die( __( 'The theme you are currently using is not compatible with Full Site Editing.' ) );
}

Copy link
Member

@fabiankaegy fabiankaegy Sep 14, 2022

Choose a reason for hiding this comment

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

@hellofromtonya I belive something like this would solve the case you uncovered in your testing here: #3234 (comment)

With the block-fragments (classic theme with block-based templates) enabled, I am able to directly access /wp-admin/site-editor.php but it's a white screen. No errors in the browser's console or error logs.

Suggested change
$is_template_part_editor = isset( $_GET['postType'] ) && 'wp_template_part' === sanitize_key( $_GET['postType'] );
if ( ! wp_is_block_theme() && ! $is_template_part_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
}

As far as I'm aware that was not a known issue and no patch for it had been created.

@Mamaduka asked me to keep an eye on this PR since he is out traveling. I will create a PR on the Gutenberg Repo to fix the issue there aswell.

Copy link
Member

Choose a reason for hiding this comment

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

Actually in looking at the code in Gutenberg it was handled as a completely separate admin page. So this issue only exists in this PR and only needs to get fixed here. No backport to Gutenberg is needed.

https://github.com/WordPress/gutenberg/blob/d19258cb5b85cea52e85a3e86c9550dd48d44fca/lib/compat/wordpress-6.1/template-parts-screen.php#L5

Copy link
Member

Choose a reason for hiding this comment

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

In order to make it easier to test, I have also created a separate PR against @Mamaduka's branch which applies the same fix as the one I have shown in the suggestion above: Mamaduka#48

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that does resolve it. Well done @fabiankaegy - thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

non block themes that support block based template parts should only be allowed to access the site-editor.php file if the query parameter to access the template part editor is set correctly
…-parts-mode-fix

fix prevent unwanted access of site-editor.php
@@ -406,7 +407,7 @@ public function test_get_item_schema() {
$this->assertArrayHasKey( 'responsive-embeds', $theme_supports );
$this->assertArrayHasKey( 'title-tag', $theme_supports );
$this->assertArrayHasKey( 'wp-block-styles', $theme_supports );
$this->assertCount( 21, $theme_supports );
$this->assertCount( 22, $theme_supports );
Copy link
Contributor

Choose a reason for hiding this comment

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

This was bumped to 22 by 72380e3 (#3205):

$this->assertCount( 22, $theme_supports );

So we'll need to rebase and increment it again:

Suggested change
$this->assertCount( 22, $theme_supports );
$this->assertCount( 23, $theme_supports );

@ockham
Copy link
Contributor

ockham commented Sep 15, 2022

Thanks a lot, @fabiankaegy! Looks like this PR is good to go, except for failing unit tests, which I think are due to this: https://github.com/WordPress/wordpress-develop/pull/3234/files#r971773508.

@Mamaduka If you're around, could you rebase and change that number to 23? 🙏

@ockham
Copy link
Contributor

ockham commented Sep 15, 2022

@Mamaduka is currently AFK -- I've forked this PR and applied my fix: #3258

@hellofromtonya
Copy link
Contributor

Thank you everyone for your contributions! Committed PR 3258 (finalization of this PR) via https://core.trac.wordpress.org/ticket/56467.

@Mamaduka Mamaduka deleted the backport/site-editor-template-parts-mode branch May 9, 2023 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants