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

[Patterns]: Remove bundled patterns #46450

Merged
merged 1 commit into from
Feb 1, 2023
Merged

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Dec 10, 2022

What?

Resolves: #46017

The GB bundled patterns have been added to Pattern Directory and tagged as core patterns with 6.1 WP version. With that we don't need to bundle them anymore.

Testing Instructions

  1. Ensure that the patterns from this comment: Ensure Header & Footer patterns use the Pattern Directory API #46017 (comment) are loaded properly.
  2. You could just disable Gutenberg in a 6.1 installation. Since we tagged them as core patterns with 6.1 version in Pattern directory, they should just show up even without Gutenberg being active.

Notes

In my testing it seems I've encountered this issue that some images fail to load. If that's the case for others as well and not something with my environment maybe we need to update some urls, etc.. in the Pattern Directory patterns?

@ntsekouras ntsekouras added the [Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced label Dec 10, 2022
@ntsekouras ntsekouras self-assigned this Dec 10, 2022
@jasmussen
Copy link
Contributor

Nice! What's a good way to test? Just looking at this PR, I still see patterns showing up in the inserter. But perhaps they are coming from the directory now?
Screenshot 2022-12-12 at 10 46 12

Happy to ✅ if that's what's up.

@ntsekouras
Copy link
Contributor Author

@jasmussen I think the best way to check would be to just disable Gutenberg in a 6.1 installation. Since we tagged them as core patterns with 6.1 version, they should just show up. (I'll also update the description)

@annezazu
Copy link
Contributor

Rather than trying to use a specific Gutenberg plugin for this PR, I followed what you said above and just disabled Gutenberg on a 6.1.1 install. At first, everything was working as expected, except the Header category included some "banner" patterns. I also noticed the navigation block was acting strangely, requiring that a new menu be created. After I did that, the patterns entirely broke:

strange.bug.mov

Using 6.1.1, no Gutenberg, TT3. It broke other pattern categories too, like Gallery, but not Buttons.

@annezazu annezazu mentioned this pull request Dec 13, 2022
57 tasks
@ntsekouras
Copy link
Contributor Author

Rather than trying to use a specific Gutenberg plugin for this PR, I followed what you said above and just disabled Gutenberg on a 6.1.1 install. At first, everything was working as expected, except the Header category included some "banner" patterns. I also noticed the navigation block was acting strangely, requiring that a new menu be created. After I did that, the patterns entirely broke:

Thanks for testing Anne! Is this something known for Pattern Directory that can be fixed? I remember similar issues before and I had the impression they were fixed.. --cc @beafialho @ryelle

@ryelle
Copy link
Contributor

ryelle commented Dec 13, 2022

the Header category included some "banner" patterns

Can you point out which patterns these are? Maybe the patterns in the pattern directory need to be recategorized? (note that we haven't implemented the new categories yet, so this might be expected)

I also noticed the navigation block was acting strangely, requiring that a new menu be created.

Looks like the pattern from the directory includes a ref, while the patterns deleted from this PR omit that. The ref'd menu 80 doesn't exist on your site, so it errors. Maybe we can strip out the ref when outputting the pattern code from the directory?

<!-- wp:navigation {"ref":80,"layout":{"type":"flex","justifyContent":"center"}} /-->

After I did that, the patterns entirely broke

I think you're still in the Navigation block, so you're restricted to what blocks you can insert. That's why you can insert the social links pattern but nothing else.

@ntsekouras
Copy link
Contributor Author

ntsekouras commented Dec 13, 2022

Looks like the pattern from the directory includes a ref, while the patterns deleted from this PR omit that. The ref'd menu 80 doesn't exist on your site, so it errors. Maybe we can strip out the ref when outputting the pattern code from the directory?

This should be omitted in the pattern directory, right? If it's not omitted there, then it can not work in any site that doesn't have a menu with that ref(with the PD flow to copy the pattern).

the Header category included some "banner" patterns

What do you mean banner? There is no such category as far as I know.

@ryelle
Copy link
Contributor

ryelle commented Dec 13, 2022

This should be omitted in the pattern directory, right?

I believe the nav block is created with a ref by default, either that or they were created in the directory with a menu (maybe because @beafialho has higher permissions than a regular user). We don't do anything to override the way the block is saved. In any case, I've already created an issue to fix this in the directory.

@annezazu
Copy link
Contributor

Can you point out which patterns these are? Maybe the patterns in the pattern directory need to be recategorized? (note that we haven't implemented the new categories yet, so this might be expected)

Here are the names:

  • Media and text in a full height container
  • Media and text with image on the right
  • Media and text with image on the left
  • Larger header with text and a button
  • Larger header with left aligned text
  • Event

@ryelle
Copy link
Contributor

ryelle commented Jan 6, 2023

@annezazu I've updated those patterns in the pattern directory, so they've been removed from the Headers category and added to the Banners.

@annezazu
Copy link
Contributor

Noting for clarity (mainly for my future self) that we're blocked right now due to this issue that remains to resolve in the pattern directory: WordPress/pattern-directory#545

@annezazu
Copy link
Contributor

What do you mean banner? There is no such category as far as I know.

I also just saw this and wanted to note the banner category being added previously: #44203

@ryelle
Copy link
Contributor

ryelle commented Jan 27, 2023

Noting for clarity (mainly for my future self) that we're blocked right now due to this issue that remains to resolve in the pattern directory: WordPress/pattern-directory#545

This has been fixed in the pattern directory, so it should be good to go 👍🏻 (there might still be old patterns in cache for a few hours, in case anyone jumps on this right away)

@ntsekouras
Copy link
Contributor Author

This has been fixed in the pattern directory, so it should be good to go 👍🏻 (there might still be old patterns in cache for a few hours, in case anyone jumps on this right away)

Thank you @ryelle! I tested and this looks good! I'd need someone else to test too and 🟢 this PR to land. -- @annezazu @jasmussen

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

It works as expected. I can see core patterns loaded with and without the Gutenberg plugin.

P.S. I noticed that some patterns like "Header with hero image" are rendered broken because the image fails to load.

@ntsekouras
Copy link
Contributor Author

P.S. I noticed that some patterns like "Header with hero image" are rendered broken because the image fails to load.

It seems we have the same issue for Cover blocks, as it was for Image. -- cc @ryelle

@ryelle
Copy link
Contributor

ryelle commented Jan 30, 2023

It seems we have the WordPress/pattern-directory#443 for Cover blocks, as it was for Image.

That issue was for the pattern creator, inserting images from the Openverse media flow, and was fixed (for all images in the openverse client).

The issue you're seeing sounds like this one, which should have been fixed with this PR. In fact, if you copy the pattern content directly from wp.org, it works. So I think there's some extra encoding/sanitization on the core side that's converting the &amp; to \u0026amp;. I think it's the wp_kses_post here.

If there's anything that should change on the API side to fix this, add a new issue here.

@richtabor
Copy link
Member

P.S. I noticed that some patterns like "Header with hero image" are rendered broken because the image fails to load.

+1.

Here's a screenshot of header patterns, loaded with Twenty Twenty Three:

CleanShot 2023-01-30 at 14 45 31

@annezazu
Copy link
Contributor

Tested and can confirm the images failing to load. Of note that they are broken after adding too. For example:

Screen Shot 2023-01-30 at 6 30 35 PM

@ntsekouras
Copy link
Contributor Author

ntsekouras commented Feb 1, 2023

I'm going to merge this for now and handle separately the link issue for Cover blocks, since these patterns are not even shown since the core keyword` has been added in PD. Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Patterns A collection of blocks that can be synced (previously reusable blocks) or unsynced [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure Header & Footer patterns use the Pattern Directory API
7 participants