Block patterns: adding blockTypes to FSE patterns

[ramonjd]

Changes proposed in this Pull Request

This PR checks a pattern's pattern_meta for block type values and adds them to the options array when registering blocks with register_block_pattern. WOOT!

This PR was brought to you by Yorkshire Gold Tea!

Testing instructions

Enable a block-based theme, e.g., TT1 blocks

Add a new Header block to a template in the Site Editor. In the "Template Part" dialogue, click "New template part"

A default pattern should appear. These patterns are fetched from the source site dotcomfsepatterns.

Check that other header/footer patterns load as well:

Unit tests:

cd apps/editing-toolkit
yarn run test:php --testsuite block-patterns

Related to https://github.com/Automattic/view-design/issues/111

[github-actions]

Test live: https://calypso.live/?branch=add/block-types-to-fse-pattern-registration

[matticbot]

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.

[andrewserong]

I probably won't get a chance to take this for a spin today (I'll give it a review on Monday), but just wanted to say from a quick glance at the code change, it looks great!

[andrewserong]

Just copy+pasting PHPCS' friendly warnings for Monday 😀

[ramonjd]

Just copy+pasting PHPCS' friendly warnings for Monday 😀

Thanks. They've been terminated.
Nothing better than stomping on the whiny linter on a Friday afternoon. :)

[Copons]

Would it make sense to rely on the pattern category to infer the blockType? 🤔

Something like this (pseudocode):

if ( $pattern['category'] === 'header' || $pattern['category'] === 'footer' ) {
  register_block_pattern(
    $pattern['name'],
    [ 'blockType' => 'core/template-part/' . $pattern['category' ]
  );
}

[ramonjd]

Would it make sense to rely on the pattern category to infer the blockType? 🤔

Thanks for the feedback!

That would be our first approach for sure. 👍

We have a system for managing WPCOM patterns, where we keep categories/tags and other meta in one place (a source site). See PCYsg-t0i-p2

I thought it would be easier for us to add other blockType meta this way in the future. It also allows us to tweak taxonomy and test things out without having to release the ETK.

cc @andrewserong also in case my thinking is off the cliff

[andrewserong]

cc @andrewserong also in case my thinking is off the cliff

Thanks for the ping! It'd be simple to hard-code it to the existing category (nice idea @Copons!), but I agree I think there's more flexibility in using pattern meta so that we don't need to tie the target block type to the existing pattern category. A couple of potential scenarios come to mind:

• If we want to show a pattern in a single category in the inserter, but have it associated with multiple block types
• If the name of the block is different to the pattern category. E.g. if we wanted use pattern categories like "Blog posts" or "Latest posts" but the underlying block we're targeting is core/query. It looks like one of the examples in Gutenberg has a pattern category of buttons but blockTypes set to core/social-links.

So I think it'd be a little bit more manual work to add in pattern meta in the source site, but should hopefully give us a bit more flexibility and result in fewer code changes if we need to move things around.

[ramonjd]

Hi @Copons and @Addison-Stavlo !

Does the manual testing text in the PR description make sense?

I wasn't sure if that's how we're supposed to test this on WPCOM.

Also, I've added core/template-part/footer and core/template-part/header to a few of our header/footer patterns and see that only one is being inserted anyway.

Does that sound normal?

If that's normal, then all good 😄

It means we can activate one default header/footer for WPCOM. This would apply to all site, unless we eventually add theme-specific default header/footer templates to our themes (?) I'm not sure, but any advice you have will be very helpful! 🙇

[Copons]

Hi @Copons and @Addison-Stavlo !

Does the manual testing text in the PR description make sense?

I wasn't sure if that's how we're supposed to test this on WPCOM.

Just tested it, and I think there's no need to add that filter in 0-sandbox?
I mean, with that filter forced true it's not working for me; without, this tests just fine. 😄
No idea why, but I think it's enough to have a relatively recent WPCOM FSE site using Core FSE, with the core-site-editor-enabled sticker.

tl;dr I could test this by simply:

• Sync the PR to the sandbox via script.
• Create a new FSE site with the Horizon flow: https://horizon.wordpress.com/new?flags=gutenboarding/site-editor
• Sandbox the site.

Also, I've added core/template-part/footer and core/template-part/header to a few of our header/footer patterns and see that only one is being inserted anyway.

Does that sound normal?

If that's normal, then all good 😄

It means we can activate one default header/footer for WPCOM. This would apply to all site, unless we eventually add theme-specific default header/footer templates to our themes (?) I'm not sure, but any advice you have will be very helpful! 🙇

Not exactly sure what you mean 🤔
I can see multiple template part variations for both Header and Footer, and can slide through them with the arrow, or show them in a grid:

I could insert both of them just fine anyway!

---

One issue (unrelated to this PR) that I've noticed is that all the Footer variations show up with a broken block.

If I edit standalone the template part, it works fine.

Apparently it's a block validation issue:

For some reasons the save function adds a --wp--style--color--link to the Group styles, or viceversa, that variable is stripped away when the block is persisted in the post. 🤔

[Addison-Stavlo]

I've added core/template-part/footer and core/template-part/header to a few of our header/footer patterns

Makes sense! Curious, where is this done? I know we have source sites, but Im not familiar with where one would go to update the pattern meta like this.

that only one is being inserted anyway. Does that sound normal?

Also not sure I follow, but it looks like the patterns are registering for their corresponding locations so nothing is jumping out at me as abnormal.

I agree I think there's more flexibility in using pattern meta so that we don't need to tie the target block type to the existing pattern category.

Also agree here. Hardcoding it to the pattern meta seems the best way to go as categories and blockTypes definitely don't translate towards each other in a lot of cases.

Apparently it's a block validation issue:

The culprit is probably in the markup for that block/pattern. I ran into something similar recently and it was due to a block attribute of type number accidentally being added as a string (ex 10px instead of 10). Could be something similar?

[Addison-Stavlo]

Does the manual testing text in the PR description make sense?

Also noting the same as @Copons :

think it's enough to have a relatively recent WPCOM FSE site using Core FSE, with the core-site-editor-enabled sticker.

I just sync'd this PR to the sandbox, and loaded up a sandboxed FSE site that I made earlier this month. Things seem to work without the add_filter thats noted in the instructions.

[creativecoder]

One issue (unrelated to this PR) that I've noticed is that all the Footer variations show up with a broken block.

@Copons @Addison-Stavlo That looks like the same problem described in p7H4VZ-30a-p2, the --wp--style--color--link variable is getting stripped from the block's inline styles.

But I thought that was fixed in D60947-code 😕

[ramonjd]

Thanks for the feedback, everyone! 🙇‍♀️

Not exactly sure what you mean 🤔
I can see multiple template part variations for both Header and Footer, and can slide through them with the arrow, or show them in a grid:
Also not sure I follow, but it looks like the patterns are registering for their corresponding locations so nothing is jumping out at me as abnormal.

Sorry for the confusion.

It was a misunderstanding on my part and I didn't look at the UI closely enough 🤦 Please ignore me.

The culprit is probably in the markup for that block/pattern. I ran into something similar recently and it was due to a block attribute of type number accidentally being added as a string (ex 10px instead of 10). Could be something similar?

I've also seen this before.

We added an issue a while back to look at this: https://github.com/Automattic/view-design/issues/269

We can bump priority.

I'll investigate the validation issue, thanks for pointing it out!

But I thought that was fixed in D60947-code 😕

We believe that the padding et. al. rules were skipping optimization thanks to that fix. Not sure about the CSS vars. Perhaps kses is stripping them out.

Update: We'd have to do something similar to D60947-code, but discard invalid properties. I'm not sure how popular that change would be.

Edit: Possibly related:
WordPress/gutenberg#31488
WordPress/gutenberg#29297

[ramonjd]

I mean, with that filter forced true it's not working for me; without, this tests just fine. 😄
No idea why, but I think it's enough to have a relatively recent WPCOM FSE site using Core FSE, with the core-site-editor-enabled sticker

That's because I forgot I turned it on for everyone in D59351-code. 😭 😆 As you were...

[ramonjd]

For some reasons the save function adds a --wp--style--color--link to the Group styles, or viceversa, that variable is stripped away when the block is persisted in the post. 🤔

I removed the link colors from the patterns for now while we work out the best way around it. Thanks again, everyone for the 👀 !!

[Addison-Stavlo]

It was a misunderstanding on my part and I didn't look at the UI closely enough 🤦 Please ignore me.

Ohhh, Yeah I see now. I think feedback on that component has been a bit rough, as you pointed out you didn't even notice the selection component was there which is a fair critique from forcing some user testing on you. 😆 I think there will be some explorations regarding converting this selection component to be a modal so it stands out a bit more from the canvas (As well as better a11y i think) WordPress/gutenberg#31033. But nothing that should affect implementation here. 😁