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

Navigation: Allow multiple navigations with the same ref #47453

Merged
merged 5 commits into from
Jan 26, 2023

Conversation

scruffian
Copy link
Contributor

@scruffian scruffian commented Jan 26, 2023

What?

Our last fix for preventing recursive navigation blocks (#46387) prevents users from having the same navigation twice in one page. This allows that use case while still preventing nested navigation blocks.

Fixes #47085 (comment)

Why?

Users should be allowed to create multiple navigation menus on the same page, with the same ref.

How?

Use a recursive loop to check whether a block or any inner blocks contain a navigation block.

Testing Instructions

  1. Try adding two navigation menus with the same ref to a post/template. You should see both of them.
  2. You can also try testing nested navigation menus by adding <!-- /wp:navigation --> to a wp_navigation CPT, but you'll need to use mysql. (See Navigation Block: Prevent circular references in navigation block rendering #46387 for instructions).

Testing Instructions for Keyboard

As above.

Screenshots or screencast

Two identical navigations on the same page:
Screenshot 2023-01-26 at 11 59 28

@scruffian scruffian added the [Block] Navigation Affects the Navigation Block label Jan 26, 2023
@scruffian scruffian marked this pull request as ready for review January 26, 2023 11:59
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Code LGTM - there's some linting. I totally support this for now. If we ever figure we need navigation blocks inside navigation blocks we can then check for ref equality as well.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Nice work. Please can we reference the bug report Issue in this PR description so we're clear what this PR is fixing?

* @param array $parsed_blocks the parsed blocks to be normalized.
* @return bool true if the navigation block contains a nested navigation block.
*/
function block_core_navigation_has_nested_core_navigation( $parsed_blocks ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function block_core_navigation_has_nested_core_navigation( $parsed_blocks ) {
function block_core_navigation_block_contains_core_navigation( $parsed_blocks ) {

Nit: I think we can just name this to explain that it's testing a set of blocks for the presence of a core navigation block. The argument is "a collection of blocks" not "a core navigation block".

It's not "does this core navigation block contain core navigation blocks".

@github-actions
Copy link

Flaky tests detected in 575db6b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4014912047
📝 Reported issues:

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 Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header Navigation menus aren't displayed when added to site footer
6 participants