Add navigation menu blocks

[jorgefilipecosta]

Description

This PR adds navigation menu blocks it is a first small step to get #13690.
It is a first small interaction using only what blocks can do right now.
It still contains the nested blocks UI.
But it creates a basis from where we can follow up in parallel.

Short term follow up tasks to get to the v1:

• Finish the inserter component as shown in Add block: Navigation menu #13690.
• Add a logic that when a navigation menu is created we automatically generate a block list based on top level pages.
• Allow InnerBlocks to be customized so we can remove the wrapping block UI.
• Expose block Drag & Drop/mover functionality so it can be customized/reused here.

Changes outside block-library done here are small improvements to make this possible but they are not related to this PR and will be extracted and properly documented.

How has this been tested?

I checked it is possible to create a menu, I checked the output markup is the expected one.

[gziolo]

It would be nice to follow #14770 and introduce block.json file for new blocks.

[Soean]

Maybe add a i18n function?

[gziolo]

How about we put this block behind the GUTENBERG_PHASE feature flag and merge it later this week? @mapk how does it look behavior wise?

Aside: Reviewing 750 lines long PR is going to be suboptimal if we continue iterating too long on this PR.

[jorgefilipecosta]

Hi @gziolo all your feedback was applied. I was in doubt if I should use hooks given that we are not using them widely on our code base. But I think it is time to start and given that we are implementing totally new blocks it seems a nice opportunity. I refactored the code to use hooks and simplified it.

[gziolo]

Code wise, it looks great. I love how those React hooks make everything simpler. After all, it isn't that much to learn after you do the homework by reading their docs and the article I referenced in my previous comment.

Love your latest changes ❤️

[gziolo]

The direction is to have all client-side specific cod in its own file. save should be moved to save.js file. This will allow to get rid of index.js completely at some point.

[gziolo]

The same note about save as for another block :)

[gziolo]

It looks like it can be removed.

[gziolo]

Can you put those blocks behind the feature flag similar to Legacy Widget?

[jorgefilipecosta]

Thank you for the code review @gziolo! The feedback was applied I think from the code point of view this getting ready.

From the design/UX point of view, we are not there yet, getting there would create an even bigger PR making it impossible to review in one go so we will need some follow up PR's.
For reference here are some images of the current behavior:

(ends with using the keyboard to navigate around all the items).

I guess there are two options:
We merge this PR with the blocks exposed as they are now (behind the phase 2 flag so they appear only on the plugin) marked as experimental.
We merge but we add an {inserter: false } flag making it impossible for anyone to notice these blocks. E.g: they don't appear anywhere unless the flag is changed.

I guess we should go with option 1 if we think these samples already provides some value and may be useful and we want testing to be easy.
We should go with option 2 if we think we are still in a very early stage and we prefer to keep the blocks hidden, making people that want to test the block need to change the flag.

Any thoughts on this @mapk, @melchoyce, @sarahmonster?

[sarahmonster]

👍 I've gone ahead and requested some re-reviewing, and if we need extra help we can surface in this weeks' core-editor chat. Then we can move on to the next stages! Sound good?

[earnjam]

I haven't tested this out yet, but on a quick pass my main concern is this completely changes the way nav menus are stored in the database. That's going to be a concern for backward compatibility.

Currently nav menus are stored as a taxonomy term, with each item stored as a post and assigned that term. I'm not sure if those would be accessible to this block once we merge a menus endpoint (@wpscholar has put together a patch that covers all CRUD operations on menus)

Even if a nav menu block is saved as a reusable block, this PR would basically store everything for that menu as a blob inside of post_content. This makes it much more difficult to access and parse from an API perspective.

It also means we'd have to have 2 different ways to return menus via the REST API depending on how they were created.

The current storage method could still allow rich block-based menus. Since individual menu items are already stored as posts, their extra block markup could be stored in post_content. I think the main thing that would need to be figured out is how to save any settings that apply to the menu as a whole.

[youknowriad]

@earnjam I think we should distinguish the menu block from the current menus. In the block editor, blocks have their data as attributes, blocks can be used in several contexts, inside post_content, soon maybe inside widget areas, and potentially later in "global templates". Blocks can also be used in a WP agnostic context.

For this reason, I think the BC concern is outside of the scope of this PR. The block is going to be used like any other block and should work in any of these contexts. Whether this block can be used in the menu screen and its attributes mapped to the existing storage is a separate question that is not impacted by this PR.

[earnjam]

@youknowriad I understand the desire for a platform agnostic menu block, but perhaps that block shouldn't be the default menu block for WordPress.

Because that means we're either excluding traditional WordPress menus from being placed into areas that blocks can be used, or we'll require another "legacy menu" block that allows them to be inserted, which is just confusing.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

[youknowriad]

I don't think we should base our future decision on what the WP menu block should be on the fact that WordPress has already existing menu concept. We should be thinking about what is the best menu block we could build for the future and then try to adapt it to work in WordPress.

Because that means we're either excluding traditional WordPress menus from being placed into areas that blocks can be used or we'll require another "legacy menu" block that allows them to be inserted, which is just confusing.

We don't need any other block, we already have the "Classic Widget" block than can be used for the existing menus.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

Why do you think so? For the future, I expect APIs to retrieve blocks composed of attributes.

[earnjam]

We don't need any other block, we already have the "Classic Widget" block than can be used for the existing menus.

That's going to be pretty confusing for users. They want to insert a menu and choose the menu block, but then can't use the menu that they've already created. It isn't intuitive at all that the Legacy Widget block should be used for inserting menus.

That also means that for the menus of the future, we're replacing a more structured storage implementation with one that is more difficult to parse. If you want to get menus via an API endpoint for some external client, then there will be multiple ways that they need to be retrieved and/or parsed.

Why do you think so? For the future, I expect APIs to retrieve blocks composed of attributes.

Let's say a site has a traditional menu and a user creates a menu using this block. How would you retrieve both of those via the REST API?

The API either needs to handle the logic of retrieving and parsing two types of data, or else there are two endpoints.

[sarahmonster]

That's going to be pretty confusing for users. They want to insert a menu and choose the menu block, but then can't use the menu that they've already created.

Agreed! This is why our original design proposal suggests the ability to, upon creating a new menu block, convert your existing menus to blocks.

...where "User selects from existing menus" looks something like this:

[youknowriad]

Let's say a site has a traditional menu and a user creates a menu using this block. How would you retrieve both of those via the REST API?

By asking the question, you assume "menus" are an important concept that needs to fetched using the REST API. This assumption is completely true today (if we omit some menus created in an adhoc way including plugins to create menus) but is this assumption valid for the future?

Potentially, In terms of content, there's mainly two/three concept that matters for the user:

• CPTs (posts, pages...)
• Blocks in the content of these CPTS
• Templates assigned to these CPTs.

Content creation happens through these-screen (editors) and based on blocks. Menus are less important and represent just another block. Third-party blocks can create custom menu blocks, custom menu items. In fact, a "Recent Post" block is not much different from a menu, it's just a menu composed of given posts/pages... A list of buttons/links aligned next to each other in templates are also menus, what matters to the user at the point is the fact that the links work as expected and that they are global and edited in a single place.

What is a menu in that case? Can you fetch recent post menu items today? Is it important for the user at that point to fetch menu items of a given block but not the other? I think it's more important for people to fetch blocks that compose templates and blocks that compose post_content at that time.

To be honest, I think we don't know all the answers to these questions. I don't think we should assume things and adapt the block editor's framework for something that we can't know for sure.

That's why I suggest that we build the best menu block we can think of UI-wise first and iteration will inform us about the best storage system, how do users would want to consume the blocks...

[earnjam]

Thanks @youknowriad I don't want to be argumentative here. I appreciate you taking the time to outline your thoughts.

I just want to make sure to voice some backward compatibility concerns. But I guess we are taking the same sort of mindset as the editor rollout. i.e. At a certain point we replace the menu screen entirely and people can just convert existing menus to new block-based menus when they go to edit them.

[youknowriad]

Thanks @earnjam for the understanding. I definitely don't want to dismiss any feedback and BC is still an important concern. It's important that we keep iterating in the plugin which will allow us to discover the shortcomings, the BC concerns and the use-cases that we may not cover properly and we'll have to adapt as these get clarified.

[sarahmonster]

Hello everyone! @jorgefilipecosta has informed me we can merge this PR without committing to a particular direction in terms of how the menu items are stored for now, and progress on the development of this block is still stalled pending getting this merged.

Let's continue the ongoing debate, but if someone has time to review this PR and get it over the finish line, we'd really appreciate that. ❤️

[spacedmonkey]

Is it know theme creators will need to do support this new block? The css can get pretty complex for menus and menus are likely to look extremeley different depending on where there are on the site. Menus in a footer would be different from in content or sidebar for example. Is there going to be a way for developers to opt in to this block? Automatically opting them in will like end up with broken blocks on the front end.

[jorgefilipecosta]

Is it know theme creators will need to do support this new block? The css can get pretty complex for menus and menus are likely to look extremeley different depending on where there are on the site. Menus in a footer would be different from in content or sidebar for example. Is there going to be a way for developers to opt in to this block? Automatically opting them in will like end up with broken blocks on the front end.

Hi, thank you for bringing these points. Right now the plan is to have a standard block and polish its UI, making improvements in the process ( to InnerBlocks for example) that any block developer can take advantage. There are ways themes can use to disable a specific block, regarding the styles the block will provide a default set of styles like other blocks do, and themes can opt-in to remove some of this styles.

This PR was rebased taking into account updates that happen meanwhile, and the changes in #14867, it should be ready to merge after #14867.

[draganescu]

I have tested locally and the blocks work in this alpha state. It works as described! Also the code itself is a stub for a lot more functionality to be added so we'll see what the final form will be.

Committing this hidden away is a step forward so we can continue with smaller, more focused PRs for each part of the navigation menu block.

@jorgefilipecosta there is though a failing "Filter merged" check , not sure yet what this is about, never seen that before, so if you could let me know or fix it we can move along 💃

[jorgefilipecosta]

Hi @andreilupu, I rebased this PR, there are no errors happening now. Before merging this PR we need to merge #14867, very simple PR just passing down a prop.

[andreilupu]

Hi @andreilupu, I rebased this PR, there are no errors happening now. Before merging this PR we need to merge #14867, very simple PR just passing down a prop.

As much as I'm honored about the @ reference, I think that the shout out was for @draganescu

I can still test the PR later if it's needed.