WordPress / pattern-directory

The WordPress Block Pattern Directory
https://wordpress.org/patterns/
GNU General Public License v2.0
119 stars 31 forks source link

Patterns using a Navigation block have `ref` set #545

Closed ryelle closed 1 year ago

ryelle commented 1 year ago

For example, Centered header with logo — the pattern code has a navigation block with a ref set, and when that's used on a different site, the referenced menu does not exist, causing an error:

Screenshot 2022-12-13 at 12 29 15 PM

Example pattern code:

<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"2em","bottom":"4em"}}}} -->
<div class="wp-block-group alignfull" style="padding-top:2em;padding-bottom:4em"><!-- wp:site-logo {"align":"center"} /-->

<!-- wp:site-title {"textAlign":"center","fontSize":"large"} /-->

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

The ref should not be there.

tellyworth commented 1 year ago

What's the solution here - strip the ref out on save, and then do a one-time update of any existing patterns with ref?

ryelle commented 1 year ago

What's the solution here - strip the ref out on save, and then do a one-time update of any existing patterns with ref?

I think so, the messy part will be making sure that doesn't break anything. Other blocks have ref (like images) but those should also be null/removed.

ntsekouras commented 1 year ago

I'm not sure how to approach this, but sharing some thoughts..

In Pattern Directory we can insert a Navigation block in the UI(as user), but with no permissions it feels broken.. Should we remove the block from the inserter there? 🤔

Would removing the ref from these patterns: https://github.com/WordPress/gutenberg/issues/46017#issuecomment-1344742302 resolve the issue for the core patterns for now?

@getdave do you have any thoughts about this, since you've worked a ton in Navigation block?

ryelle commented 1 year ago

In Pattern Directory we can insert a Navigation block in the UI(as user), but with no permissions it feels broken.. Should we remove the block from the inserter there? 🤔

If the nav block is hidden from the inserter, then no one can use it (including the wporg user) and it can't appear in the core/bundled patterns (because we would also need to validate patterns to remove it, since some pattern authors build elsewhere and paste in content). When the directory launched, a bunch of site editing blocks were disabled including Navigation, and it was considered a blocker for GB bundled patterns - see https://github.com/WordPress/pattern-directory/issues/489, p9ue0y-Sc.

Would removing the ref from these patterns: https://github.com/WordPress/gutenberg/issues/46017#issuecomment-1344742302 resolve the issue for the core patterns for now?

That would solve the immediate blocker for GB, yeah - how would you remove it (in a way that wouldn't come back if the pattern needs to be edited)?

getdave commented 1 year ago

Since 6.1, the Nav block should not require a ref to look "correct". It has various fallbacks to ensure some content is displayed (in the following order):

Therefore I'd suggest testing that removing the ref provides a suitable experience.

Note also that ref is currently an ID reference. In the future we expect to migrate that to a slug, from which we will look up the most suitable menu (if you have one).

I hope that helps? Feel free to loop me in and I'll help as I can.

miksansegundo commented 1 year ago

@getdave I think the issue is that the editor always adds the ref when the pattern is saved. I haven't found a way to save an empty Navigation Block without the ref id as suggested in this comment by Matias.

Patterns that aim to display whatever is the user's main menu should leave an empty navigation block (no id, no inner blocks); patterns that have a very specific layout should use Link Item at will with whatever placeholders make the most individual sense.

An option is to find and remove the ref on the server side before saving the pattern for the case of creating/editing patterns in the Pattern Directory or other sources like mine.

Other alternatives are adding a Page List or static Navigation Link within the Navigation Block. I have been using static navigation links in the navigation blocks used in a collection of header patterns in our project to ensure that the previews are uniform.

I think the preview of navigation blocks in the pattern inserter in Gutenberg should have some defaults or be configurable in some way to help promote patterns before they are inserted in the editor view. What do you think?

richtabor commented 1 year ago

I think the issue is that the editor always adds the ref when the pattern is saved.

@miksansegundo Have you tried recently?

I was just able to publish a pattern with a navigation block, without ref applied. When applied to a page, the navigation menu falls back to the page list, as expected.

Here's the pattern markup, copied from .org:

<!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"var:preset|spacing|80","right":"var:preset|spacing|80","bottom":"var:preset|spacing|80","left":"var:preset|spacing|80"},"blockGap":"var:preset|spacing|60"}},"backgroundColor":"black","textColor":"white","layout":{"type":"constrained"},"fontSize":"medium"} -->
<div class="wp-block-group alignfull has-white-color has-black-background-color has-text-color has-background has-medium-font-size" style="padding-top:var(--wp--preset--spacing--80);padding-right:var(--wp--preset--spacing--80);padding-bottom:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--80)"><!-- wp:image {"align":"center","id":null,"width":96,"height":96,"sizeSlug":"full","linkDestination":"none","className":"is-style-rounded"} -->
<figure class="wp-block-image aligncenter size-full is-resized is-style-rounded"><img src="https://images.rawpixel.com/image_1300/cHJpdmF0ZS9sci9pbWFnZXMvd2Vic2l0ZS8yMDIyLTExL2xyL2xvYzIwMTc2NDU1MDktaW1hZ2UuanBn.jpg" alt="" width="96" height="96" /></figure>
<!-- /wp:image -->

<!-- wp:paragraph {"align":"center"} -->
<p class="has-text-align-center">First, Last</p>
<!-- /wp:paragraph -->

<!-- wp:navigation {"layout":{"type":"flex","justifyContent":"center"}} /--></div>
<!-- /wp:group -->
miksansegundo commented 1 year ago

@richtabor I tried in Gutenberg v14.9.1 and v15.0.0-rc.1 in WordPress 6.1.1.

In my project, I create patterns as posts on a WordPress site using Gutenberg. I'll open an issue there. When my patterns with navigation are applied to a page, I get the same error as in this issue because of the ref.

I just tried in the Pattern Directory and got an error because I don't have permission to add navigation blocks.

Screenshot 2566-01-18 at 15 03 08

https://user-images.githubusercontent.com/1881481/213118193-3bd3c4a2-c252-4c03-a590-cdce9f532031.mov

richtabor commented 1 year ago

In my project, I create patterns as posts on a WordPress site using Gutenberg.

Same here. I created that one on the latest pattern directory directly.

I just tried in the Pattern Directory and got an error because I don't have permission to add navigation blocks.

Same, though it does still show up in the pattern itself. Not ideal.

miksansegundo commented 1 year ago

@richtabor I'm guessing that you were able to create that pattern in the PD without ref because of the error Classic menu import failed, and that's why the Menu dropdown keeps loading. See it on the Settings sidebar:

Screenshot 2566-01-18 at 15 55 52
getdave commented 1 year ago

This use case suggests there's value in allowing users to toggle off the auto creation of the Nav Menu posts which back the data for the Nav block (that;s what the ref references).

I can see how the current situation is not ideal.

draganescu commented 1 year ago

I think patterns should never contain a ref because ref is specific to one site. Pattern previews of the navigation block should work with placeholder content that is replace with the block's fallback when the pattern is inserted.

getdave commented 1 year ago

@draganescu I believe we don't currently have placeholder content in the block. So my understanding of what you're saying is that we need to add a placeholder content so in the future patterns can display without ref or relying on the block's fallbacks.

However in the editor canvas how would the block know when to use the fallbacks mechanic vs use placeholders?

draganescu commented 1 year ago

@getdave yes, we should add that, if the example attribute is not enough, because there has to be a way for a pattern to preview a "default" state in the directory but fallback to user's data when inserterd. Controlled inner blocks should only be used if the pattern is overly specific (e.g. needs exactly 4 links).

However in the editor canvas how would the block know when to use the fallbacks mechanic vs use placeholders?

We should maybe expand the example attribute or add a new one?