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

Provide suitable frontend fallback for the Navigation block when no Menu is selected #36721

Closed
getdave opened this issue Nov 22, 2021 · 11 comments · Fixed by #36740
Closed

Provide suitable frontend fallback for the Navigation block when no Menu is selected #36721

getdave opened this issue Nov 22, 2021 · 11 comments · Fixed by #36740
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Status] In Progress Tracking issues with work in progress

Comments

@getdave
Copy link
Contributor

getdave commented Nov 22, 2021

What problem does this address?

When the Navigation block is inserted there is a setup/placeholder state that requires the user to select a Menu to be used in the Navigation.

The result of this is that it is possible to insert a Navigation block and not have a Menu selected.

The most important scenario where this might occur is when switching Themes. As we cannot guarantee the Navigation block will be preserved on Theme switch (especially if Nav Areas are omitted from 5.9) it is entirely possible that we might end up in a state where a Navigation block exists in the content but it has no menu assigned to it.

Currently in this scenario the block will render nothing on the front end. This isn't great. Imagine if the Navigation block is in your header and then you switch Themes. Suddenly the front of your site is "broken" because there is no Navigation block.

What is your proposed solution?

In the case that no menu is assigned to a Nav block we should provide a suitable fallback on the site front end (only). Suggestions:

  • list of the first 4 top level pages.
  • show the first wp_navigation Post (if available)
  • something else...?

Let's follow the pattern set in Core by wp_nav_menu as closely as possible.

the function displays
the menu matching the ID, slug, or name given by the menu parameter;
otherwise, the first non-empty menu;
otherwise (or if the menu given by menu is empty), output of the function given by the fallback_cb parameter (wp_page_menu(), by default);
otherwise nothing.

To be clear, it's absolutely fine to have the editor in a "setup state" (i.e. requiring user interaction) but the front end should render something to avoid "broken" states on the front of the website.

@getdave getdave added the [Block] Navigation Affects the Navigation Block label Nov 22, 2021
@getdave getdave self-assigned this Nov 22, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 22, 2021
@mtias
Copy link
Member

mtias commented Nov 22, 2021

List of top x pages might feel arbitrary and probably broken in some cases (why isn't my fifth page showing?). Using a saved wp_navigation or a saved wp_nav_menu seems better. This one seems we should keep an eye on feedback during the beta period.

@getdave
Copy link
Contributor Author

getdave commented Nov 22, 2021

How about we:

  • prefer picking a wp_navigation if one exists
  • fallback to Page List

My understanding is that it's better to render something but I'm curious to know if that isn't what folks are expecting.

I think either way it's going to be feel fairly arbitrary but possibly better than having no navigation rendered at all.

@getdave
Copy link
Contributor Author

getdave commented Nov 22, 2021

Ok I have a followup PR in #36740 which

  • prefers using the first non-empty Navigation Post as the fallback.
  • falls back to the Page List.

This mirrors what wp_nav_menu() does.

@mtias
Copy link
Member

mtias commented Nov 22, 2021

What to render also depends on how the template / pattern is built. If it uses specific link placement, the fallback should be more granular. That's where retrieving a single page could be fine.

@fklein-lu
Copy link
Contributor

@getdave @mtias This approach does not mirror what wp_nav_menu() does, because this function allows theme developers to buy out of if the wp_page_menu() fallback. Which every theme developer that knows his craft does.

The first and main reason is that outputting a list of pages in today's WordPress makes zero sense. Maybe this was good when WordPress was still mainly a blog, and pages were the new and shiny.

See Twenty Twenty-One, Twenty Twenty, Twenty Nineteen, Twenty Seventeen, Twenty Sixteen to witness how even Core doesn't use the page menu fallback.

The second reason is that wp_nav_menu() and wp_page_menu() have different function signatures, and output different markup. So it's a nightmare to support both in a theme, especially because as I said having a page menu was maybe cool 10 years ago.

The third issue, and this one is with the PR, is that the code naively assumes that the Page List block is available, which might not very well be. Blocks can be unregistered, or removed from the inserter. Either way this means developers don't want the block to be used, so you can't just fallback to the Page List assuming it is a desired fallback.

Finally I wonder if this isn't putting the cart before the horse. Surely I should be able to place a Navigation block without any menu items? Or how else am I going to be able to indicate to users where to place a menu?

@fklein-lu
Copy link
Contributor

Also I looked deeper at the PR, and just taking the first non empty wp_navigation post seems questionable at best. I'd rather have no menu then some random menu.

Nothing is worse than software doing something you didn't want to do. And just willy-nilly displaying whatever menu happens to be pulled from the database is definitely software taking decisions it shouldn't take.

@getdave
Copy link
Contributor Author

getdave commented Nov 24, 2021

Also I looked deeper at the PR, and just taking the first non empty wp_navigation post seems questionable at best. I'd rather have no menu then some random menu.

Nothing is worse than software doing something you didn't want to do. And just willy-nilly displaying whatever menu happens to be pulled from the database is definitely software taking decisions it shouldn't take.

Update: I missed your comment here #36721 (comment). Let me come back to you.


Thanks for your feedback @fklein-lu. I understand why you say that. However, the fallback pattern implemented here is already in place in WP Core for classic Themes that use wp_nav_menu.

See https://developer.wordpress.org/reference/functions/wp_nav_menu/#more-information

The functionality will go into WordPress 5.9 Beta 1 (release schedule here) at which point folks can test it and provide feedback. If folks want us to drop this behaviour for Full Site Editing and deviate from the established pattern then we can look at this again. Nothing is set in stone yet.

Thanks again for taking the time to provide this feedback.

@getdave
Copy link
Contributor Author

getdave commented Nov 24, 2021

@getdave getdave reopened this Nov 24, 2021
@fklein-lu
Copy link
Contributor

fklein-lu commented Nov 24, 2021

However, the fallback pattern implemented here is already in place in WP Core for classic Themes that use wp_nav_menu

@getdave So how can I as a theme developer disable this fallback in a block-based theme?

@getdave
Copy link
Contributor Author

getdave commented Nov 25, 2021

However, the fallback pattern implemented here is already in place in WP Core for classic Themes that use wp_nav_menu

@getdave So how can I as a theme developer disable this fallback in a block-based theme?

You can't...yet. We will add a hook to allow for that and/or the ability to filter what is rendered as a fallback.

Update: here is a PR for that #36850

@getdave
Copy link
Contributor Author

getdave commented Nov 26, 2021

With these complete I'm closing this one out as done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants