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

Refactor of navigation block rendering using location attribute #33244

Merged
merged 7 commits into from
Jul 7, 2021

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Jul 7, 2021

Description

Alternative to #33224

The current way the navigation block renders content using its __unstableLocation attribute has some issues:

  • It depends on the theme opting in to block-nav-menus. This is a theme option that controls the navigation screen. The block should ideally be portable and not have a dependency on how the experimental nav screen works. __unstableLocation should just work without some other configuration.
  • It short circuits the navigation block's normal rendering by instead calling wp_nav_menu, which then eventually (because of the theme opt-in) calls back to render_block( $navigation_block ), meaning the same block rendering code is executed twice, and without some care infinite recursion can result (Remove the __unstableLocation attribute prior to rendering #33175)

This PR is a bit of a refactor of how it works. The navigation block is instead in charge of its own rendering. It pulls data from a specified menu location and parses the items into blocks, replacing the normal block's innerBlocks with the parsed menu items instead.

It's just a first iteration, but I think the good this is that this could lead to changing how the navigation screen works. That screen can now just render a navigation block with an __unstableLocation attribute when a theme has opted in, and instead have the block do all the work. I'll follow-up with a PR that does this.

How has this been tested?

(I tested this using Twenty Twenty, which has a footer location and the specified colors)

  1. Create a menu using the appearance > menu screen, set it to the footer location
  2. Add a navigation block to a post by pasting the following snippet into the code editor:
<!-- wp:navigation {"orientation":"horizontal","textColor":"accent","backgroundColor":"subtle-background","__unstableLocation":"footer","style":{"typography":{"textTransform":"uppercase"}}} /-->
  1. Save and view the post

Expected: The menu should render with the correct menu items and navigation block settings (color, typography etc) should work.

Screenshots

Screenshot 2021-07-07 at 3 41 52 pm

Types of changes

Refactor

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@talldan talldan added the [Block] Navigation Affects the Navigation Block label Jul 7, 2021
@talldan talldan requested review from draganescu and scruffian July 7, 2021 07:45
@talldan talldan self-assigned this Jul 7, 2021
@talldan talldan requested a review from ajitbohra as a code owner July 7, 2021 07:45
@talldan talldan changed the title Refactor of navigation block rendering Refactor of navigation block rendering using location attribute Jul 7, 2021
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.

Yes. This is:

a. moving things forward for the navigation block in terms of interop with classic menus
b. preserving the independence of the new navigation editor from the block and the other way around, so that we can safely experiment without breaking things
c. probably most important, provides for people building block themes today a way to use classic menus and a path for a later "migration" mechanism.

if ( empty( $location ) ) {
return false;
return;
Copy link
Member

Choose a reason for hiding this comment

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

I think it might better to return an empty array when "bailing" early in this function. This way, we can have a single return type.

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.

I think like this direction and agree with @draganescu.

The wp_nav_menu can be filtered and have custom Nav Menu Walkers. It can be hard to account for all the scenarios/markup.

Tested with 2020, and it worked as expected 👍

One thing that was a little confusing is that the Navigation block displays a placeholder state. When __unstableLocation attribute is used.

@draganescu draganescu merged commit 123debc into trunk Jul 7, 2021
@draganescu draganescu deleted the try/render-navigation-from-location-refactor branch July 7, 2021 15:45
@github-actions github-actions bot added this to the Gutenberg 11.1 milestone Jul 7, 2021
@talldan talldan added the [Type] Experimental Experimental feature or API. label Jul 8, 2021
sarayourfriend pushed a commit that referenced this pull request Jul 15, 2021
* Return result straight from render_menu_from_location

* Try converting menu items to WP_Blocks

* Pass context

* Refactor into separate functions

* Add doc blocks

* Fix typo

* Return an empty string
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 [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants