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 using a Navigation block have ref set #545

Closed
ryelle opened this issue Dec 13, 2022 · 14 comments · Fixed by #550
Closed

Patterns using a Navigation block have ref set #545

ryelle opened this issue Dec 13, 2022 · 14 comments · Fixed by #550
Assignees
Labels
[Component] Patterns Issues in the patterns themselves.

Comments

@ryelle
Copy link
Contributor

ryelle commented Dec 13, 2022

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
Copy link

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
Copy link
Contributor Author

ryelle commented Jan 2, 2023

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
Copy link

ntsekouras commented Jan 17, 2023

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: WordPress/gutenberg#46017 (comment) 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
Copy link
Contributor Author

ryelle commented Jan 17, 2023

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 #489, p9ue0y-Sc.

Would removing the ref from these patterns: WordPress/gutenberg#46017 (comment) 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
Copy link

getdave commented Jan 18, 2023

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):

  • If there is a Nav Menu (wp_navigation post) then display that (or the latest one).
  • If there is a Classic Menu then display that.
  • If there are Pages then display those.

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
Copy link

miksansegundo commented Jan 18, 2023

@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
Copy link
Member

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
Copy link

miksansegundo commented Jan 18, 2023

@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

Adding.a.pattern.with.navigation.in.the.PD.mov

@richtabor
Copy link
Member

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
Copy link

@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
Copy link

getdave commented Jan 18, 2023

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
Copy link

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
Copy link

getdave commented Jan 19, 2023

@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
Copy link

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Patterns Issues in the patterns themselves.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants