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

Expose the Patterns menu to the Classic theme #5201

Closed

Conversation

t-hamano
Copy link

@t-hamano t-hamano commented Sep 13, 2023

Trac ticket: https://core.trac.wordpress.org/ticket/58827

Testing Instructions

  • In all theme variations, make sure that the appropriate submenus are displayed in the Appearance menu.
  • Confirm that access is granted or denied as appropriate when accessing each page of the Site Editor.

Block Theme

  • Activate Twenty Twenty-Three

block-theme

Block theme with customizer

  • Activate Twenty Twenty-Three
  • Create functions.php in the theme directory and add the following code:
    <?php 
    add_action( 'customize_register', '__return_true' );

block-theme-with-customizer

Classic Theme

  • Activate Twenty Twenty-One

classic

Classic Theme with block-template-parts support

  • Activate Twenty Twenty-One
  • Add code to functions.php to opt-in to block template part support
    function twenty_twenty_one_setup() {
    	add_theme_support( 'block-template-parts' );
    	// ...
    }

hybrid-theme


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.

Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link

@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 is mostly testing as advertised for me @t-hamano.

I ran into an issue though when testing Twenty Twenty One with block-template-parts support added. I get the "The theme you are currently using is not compatible with the Site Editor." error.

Comment on lines 30 to 34
if ( ! current_theme_supports( 'block-template-parts' ) && $is_template_part_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
} elseif ( ! $is_patterns_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
}
Copy link
Member

Choose a reason for hiding this comment

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

Oops, I think this is blocking classic themes with block-template-parts support to access the template part editor. I think we can adjust it to:

Suggested change
if ( ! current_theme_supports( 'block-template-parts' ) && $is_template_part_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
} elseif ( ! $is_patterns_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
}
if ( ! current_theme_supports( 'block-template-parts' ) || ! $is_template_part_editor && ! $is_patterns_editor ) {
wp_die( __( 'The theme you are currently using is not compatible with the Site Editor.' ) );
}

Choose a reason for hiding this comment

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

This fix is almost identical to what I tried locally. With this applied, I can now access all the links as advertised in the PR description.

Copy link
Author

Choose a reason for hiding this comment

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

if ( ! current_theme_supports( 'block-template-parts' ) || ! $is_template_part_editor && ! $is_patterns_editor ) {

In the case of this conditional statement, I think classic themes that do not support block-templates-parts will not be able to access the Patterns page. I think I was able to control all cases correctly with the update using 879cb3b.

Copy link

@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 now however I think there is some test code we need to remove before it's ready to land.

src/wp-content/themes/twentytwentyone/functions.php Outdated Show resolved Hide resolved
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
@t-hamano
Copy link
Author

t-hamano commented Sep 20, 2023

Please let me know if there are any other considerations or potential issues in order to ship this patch and Gutenberg PR Gutenberg PR with WordPress 6.4. Should we also ask someone on the core team for feedback?

@aaronrobertshaw
Copy link

Should we also ask someone on the core team for feedback?

That would be a wise move I think. I'm caught up on other patterns issues/PRs at the moment but will try and organise another review for this one.

Thanks for the nudge 👍

@aaronrobertshaw
Copy link

Please let me know if there are any other considerations or potential issues in order to ship this patch and WordPress/gutenberg#54066 with WordPress 6.4. Should we also ask someone on the core team for feedback?

I believe this patch is fine and good to go.

The Gutenberg PR though still has an unaddressed issue that I'd flagged previously. The command palette change in that PR needs reverting so that the user is taken to the same location as the links introduced in this patch for classic themes.

@t-hamano
Copy link
Author

t-hamano commented Sep 20, 2023

Update: The Gutenberg PR that corresponds to this core patch is actually #54422. Sorry for causing confusion.

Once this core patch is applied, the classic theme can access the Site Editor's Patterns page by typing site-editor.php?path=/patterns directly. #54422 fixes unintended behavior in such cases.

@aaronrobertshaw
Copy link

aaronrobertshaw commented Oct 3, 2023

Just noting here that this enhancement has been slated for WP 6.5 as it missed the 6.4 beta
(See https://core.trac.wordpress.org/ticket/58827#comment:9)

@getdave
Copy link
Contributor

getdave commented Jan 18, 2024

👋 Hello team. Whats the update on this feature for WP 6.5? Is it likely to land and if so which features (as originally outlined in the roadmap) will make it. Thanks in advance 🙏

@t-hamano
Copy link
Author

By merging this PR, the following will be achieved in the classic theme as of WP6.5.

  • Internally allows classic themes direct access to the Site Editor's pattern page (/wp-admin/site-editor.php?path=%2Fpatterns)
  • Add a link pointing to the pattern post type list page (/wp-admin/edit.php?post_type=wp_block) under Appearance -> Patterns for classic themes.

More details can be found in this comment.

@aaronrobertshaw
Copy link

@t-hamano's comment covers nicely what will be delivered via this PR. I believe this should be good to go but we might want a quick sanity check review and approval with fresh eyes before doing so.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Copy link
Member

@fabiankaegy fabiankaegy left a comment

Choose a reason for hiding this comment

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

I tested this and it works as expected. I think this is a great feature and I don't see any blockers for it to go into WordPress 6.5.

It's approved from my end 👍

@getdave
Copy link
Contributor

getdave commented Feb 6, 2024

I reached out for clarification on this feature in WP Slack as I've heard different things about what the actual experience will be for users of WP 6.5.

Copy link

github-actions bot commented Feb 6, 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.

Core SVN

Core Committers: Use this line as a base for the props when committing in SVN:

Props wildworks, kevin940726, aaronrobertshaw, fabiankaegy, swissspidy, get_dave, kebbet.

GitHub Merge commits

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: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: kevin940726 <kevin940726@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: getdave <get_dave@git.wordpress.org>
Co-authored-by: kebbet <kebbet@git.wordpress.org>

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

Copy link
Member

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Just did some testing and it seems to work fine.

This just exposes a page in the menu that was already accessible anyway.

@kebbet
Copy link

kebbet commented Feb 6, 2024

Just tested the PR. It does not work as expected.

My classic theme uses remove_theme_support( 'core-block-pattern' ) to disable patterns. I therefor expected the menu to be hidden. Now the menu is still visible with patterns disabled.

@fabiankaegy
Copy link
Member

@kebbet the theme support you are removing is only removing the core pattern library. It does not prevent users from being able to create pattern using the UI, nor does it remove any patterns added via plugins or themes.

I'm not aware of any method to remove the patterns functionality as a whole.

So I don't think what you are describing is a blocker for merging this at it is unrelated.

I would recommend you create a new issue for adding the ability to disable patterns altogether if that is what you are hoping to accomplish.

@getdave
Copy link
Contributor

getdave commented Feb 6, 2024

@youknowriad Based on x2 approving reviews can we commit this one?

@youknowriad
Copy link
Contributor

Commit https://core.trac.wordpress.org/changeset/57543

@youknowriad youknowriad closed this Feb 6, 2024
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
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants