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

Editing Toolkit / Patterns: Support skipping registering patterns with a requires-align-wide tag #47571

Merged
merged 2 commits into from Nov 23, 2020

Conversation

andrewserong
Copy link
Member

@andrewserong andrewserong commented Nov 19, 2020

This PR is designed to skip registering patterns that require full width support, when the site's currently active theme doesn't support full width. It will help avoid the issues described in #47565 and #46215.

Changes proposed in this Pull Request

  • When registering block patterns via the API, parse the pattern's tags, and for any tags beginning with requires-, use the remainder of the tag to look up theme features. If the theme does not support the feature, then skip registering the pattern.
  • For example, we should skip registering patterns with the tag requires-align-wide on themes that don't support align-wide.

Screenshots

In a theme that supports align-wide we should still see the images-2 pattern under the Gallery category in the inserter:

The following screenshot is from a test site using the Maywood theme, which has support for align-wide:

image

In a theme that does not support align-wide we should not be able to see the images-2 pattern under the Gallery category in the inserter.

The following screenshot is from a test site using the Hemingway Rewritten theme, which does not have support for align-wide. Note that in the screenshot I'm highlighting the pattern below the one in the above screenshot, showing that it no longer appears:

image

Testing instructions

How to test
  1. In apps/editing-toolkit run yarn dev --sync to sync to your sandbox
  2. In your sandbox's 0-sandbox.php file add the following to ensure you're using fresh results from the patterns API that include the appropriate tags:
define( 'WP_DISABLE_PATTERN_CACHE', true );
  1. With your test site sandboxed, and using a theme with align-wide support (e.g. any of the suggested themes / Varia child themes such as Maywood), go to edit a post or page, click the inserter and in the Patterns tab, under the Gallery category you should see the pattern with the title "Images" and the text within the pattern will include "5am, the beach". It should look like the top-most screenshot in this PR.

  2. Switch your theme to one that does not support align-wide (e.g. Hemingway Rewritten) and open up a post. In the inserter, under the Gallery category, you should not be able to see the pattern in question ("Images")

Fixes #47565

@matticbot
Copy link
Contributor

@andrewserong andrewserong self-assigned this Nov 19, 2020
@andrewserong andrewserong added [Goal] WPCOM Patterns Toolkit Attached to issues and PR related to our block patterns translations project. [Feature] Block Patterns Pattern content itself, and the functionality that lets you create patterns. labels Nov 19, 2020
@andrewserong andrewserong requested a review from a team November 19, 2020 06:13
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 19, 2020
@andrewserong andrewserong requested a review from a team November 19, 2020 06:14
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D53001-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

* @return bool
*/
private function can_register_pattern( $pattern ) {
if ( $this->skip_full_width_patterns && in_array( 'requires_align_wide', array_keys( $pattern['tags'] ), true ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Question: if we're expecting a specific tag anyway, might a future optimization be (assuming they'll be more of such theme exceptions) something like requires_{ $feature }, and then

	private function can_register_pattern( $pattern ) {
        foreach( $pattern['tags'] as $tag ) {
            $exploded_tag = explode( 'requires_', $tag );
            if ( $exploded_tag[1] ) {
                return false !== get_theme_support( $exploded_tag[1] )
            }
        }
		return true;
	}

Very willing to be accused of over-thinking things! :D

Copy link
Member Author

Choose a reason for hiding this comment

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

@ramonjd I actually quite like that. I'll have a play with it now, because it seems fairly safe and will also future proof it for any similar situations (the issue can be resolved by adding / removing tags, rather than requiring further code changes). Initially I thought it'd be better to cache the get_theme_support feature rather than call it on every pattern (since we'll have > 100 patterns), but from looking at how get_theme_support is implemented, it's an associative array lookup rather than looping over anything, so seems like it shouldn't be too expensive.

It just means we'd use the tag requires-align-wide instead of requires_align_wide so that we're hyphenating correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Initially I thought it'd be better to cache the get_theme_support feature rather than call it on every pattern (since we'll have > 100 patterns),

Ah yeah, good point. That totally escaped me.

Anyway, it's just an idea seeing as we're expecting a specific string regardless.

Sorry if I threw an implementation pepper into the soup!

Copy link
Member Author

Choose a reason for hiding this comment

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

No, no, it's a good idea, thank you! Better to spend a little bit longer getting it right now, so that we can save some time further down the track 😀

@andrewserong
Copy link
Member Author

Thanks for the feedback @ramonjd! I've updated this to use the simplified approach you suggested that also supports arbitrary features based on the tag's slug. I went with preg_split instead of explode so that we can match on the beginning of the slug.

I've also updated the patterns to use a requires-align-wide tag for the main problematic pattern, so this PR can now be tested, so it is now ready for a review 😀

@andrewserong andrewserong changed the title Editing Toolkit / Patterns: Support skipping registering patterns with a requires_align_wide tag Editing Toolkit / Patterns: Support skipping registering patterns with a requires-align-wide tag Nov 20, 2020
@sarayourfriend sarayourfriend changed the base branch from master to trunk November 20, 2020 16:10
Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

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

Nice job! Works according to test instructions

Maywood
Pattern is full-width

Screen Shot 2020-11-23 at 11 03 10 am

Hemingway Rewritten
Pattern does not appear in the Patterns Inserter
Nov-23-2020 11-05-59

@andrewserong andrewserong merged commit 65a2271 into trunk Nov 23, 2020
@andrewserong andrewserong deleted the fix/hide-patterns-requiring-full-width-alignment branch November 23, 2020 00:53
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 23, 2020
@ramonjd ramonjd mentioned this pull request Nov 23, 2020
11 tasks
andrewserong pushed a commit that referenced this pull request Nov 23, 2020
andrewserong pushed a commit that referenced this pull request Nov 23, 2020
* Bump etk version to 2.8.7

* Add release notes

* Updated coming soon changes from #47563

* Added notes for #47571

Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Patterns Pattern content itself, and the functionality that lets you create patterns. [Goal] WPCOM Patterns Toolkit Attached to issues and PR related to our block patterns translations project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Patterns Toolkit: should we hide certain patterns if theme features aren't available
3 participants