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 Block: save data to a custom post type #34612

Closed
Tracked by #35521
talldan opened this issue Sep 7, 2021 · 22 comments · Fixed by #35746
Closed
Tracked by #35521

Navigation Block: save data to a custom post type #34612

talldan opened this issue Sep 7, 2021 · 22 comments · Fixed by #35746
Assignees
Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.

Comments

@talldan
Copy link
Contributor

talldan commented Sep 7, 2021

What problem does this address?

One of the aspects of the navigation block discussed in the past has been how it saves its data.

In the past WordPress menus championed reusability, you can assign one menu to multiple locations on a site. When the navigation block was first envisaged, there was a discussion about whether the menu storage system (custom post types for menu items and I believe a 'term' for the menu itself) could be used, but it was decided that a more block-y approach should be taken, preferring the raw HTML output of blocks.

Since then, the way reusable blocks and template parts work has come along way, and it might be time to revisit whether the navigation block could use the same sort of system. This would essentially allow the user to create a menu once and reuse it anywhere on their site, with the blocks potentially being editable from a standalone editor

What is your proposed solution?

Use a similar implementation as template parts and reusable blocks for the navigation block.

Technical complications

I did a quick assessment of what would be required.

Reusable blocks and template parts are wrappers that save all of their inner blocks to a custom post type. The main difference to the navigation block is that the navigation block itself renders a <nav> wrapper and has a considerable number of attributes that also need to be saved and loaded, and from what I can tell, this is a new area yet to be technically explored.

An option would be to create a new wrapping block, but it would have to be interface-less.

Opportunities

The way the 'theme opt-in' in the navigation editor works still needs to be shaped considerably (#33969). There's a potential opportunity that when a theme opts-in to support the navigation block in the nav editor, the editor saves and loads the same custom post type system as the block.

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block [Feature] Full Site Editing labels Sep 7, 2021
@getdave
Copy link
Contributor

getdave commented Sep 9, 2021

Let's agree that portability of your navigations is absolutely critical for the success of the new navigation paradigms in Gutenberg. This has to be solved.

Having also looked into reusable blocks and the way content is saved I believe I'm correct in saying that core/block (Reusable block) is only there for editor UI purposes (ie: it's a utility wrapper). It's never saved.

Rather as you point out the innerblocks of the Reusable block are saved to a custom post which is dedicated to reusable blocks (ie: it's not a template part).

The Nav block is different in that - as you say - we need to say the block itself. Therefore we'd need to adapt the persistence mechanic to serialize the [Nav] block. As far as I can see that might be something like this (the following is pseudo code):

const navBlock = {
    title: title || __( 'Untitled Nav block' ),
    content: serialize(
        registry
            .select( blockEditorStore )
            .getBlocksByClientId( navBlockClientId )
    ),
    status: 'publish',
};

const updatedRecord = await registry
    .dispatch( 'core' )
    .saveEntityRecord( 'postType', 'wp_nav', navBlock );

Then when a new nav block is inserted you get the option to select from existing stored Nav blocks. If you select one then we replace the [fresh] nav block you just inserted with the one from the database. That might avoid the need for a wrapper.

In terms of the Nav Editor, as you say we could allow Themes to opt in to both block rendering and persistence. This would be a lossy action which would decouple the navigation being edited from the existing Menus post type and therefore we'd need to be pretty clear in the UI if/when navigations are "converted". This would be a nice "on ramp" for Site Editing as menus created in this way could be utilised within the Site Editor. Moreover, it would also allow the Nav Editor to become a "isolated view" for any given Navigation if it becomes too complex to edit it within the Site Editor UI space.

@talldan
Copy link
Contributor Author

talldan commented Sep 9, 2021

Also thinking some more about the other option. With a parent block type, perhaps all the controls are absorbed by the child navigation block like #26313. That actually seems close to being complete now.

Some more work could make it so that the parent block is invisible/unselectable.

I think this could be worth exploring, because a generic custom post type block with variations for saving different types of entities sounds like a nice composable pattern.

@getdave
Copy link
Contributor

getdave commented Sep 9, 2021

I see. So we need to explore:

  1. Creating a generic "transparent" (ie: no UI) custom post type block and using that to persist the entire Nav block (this would be similar to how Reusable blocks currently works but without the perceivable UI and with all the block's controls made available on the wrapped Nav block.
  2. Adapting and copying the mechanic used for Reusable blocks to perisist the entire Nav block to a custom post type. This is as described in my comment.

Did I get that right?

@talldan
Copy link
Contributor Author

talldan commented Sep 9, 2021

Yep, I think they are the two potential avenues.

@talldan talldan self-assigned this Sep 29, 2021
@talldan
Copy link
Contributor Author

talldan commented Sep 29, 2021

The comment here - #34496 (comment) - makes it clear that the separation between block wrapper and inner blocks is actually desired, and I did notice that template parts currently work that way too. It seems like that makes this easier.

I'll start exploring the much simpler solution of making the block save its inner blocks to a custom post type. 😄

@adamziel
Copy link
Contributor

adamziel commented Sep 29, 2021

I'll start exploring the much simpler solution of making the block save its inner blocks to a custom post type. 😄

Perhaps we could lean on multi-entity save from FSE here.

@carlomanf
Copy link

Template parts don't currently work in exactly the same way as traditional menus. That is, the template part block requests an explicit template part post, rather than requesting a "theme location" and rendering whichever entity happens to be assigned to that location. #31971 actually seeks to change that behaviour to fully match the way that menus work, which could in theory make menus and template parts virtually interchangeable and create a smooth transition path from menus to template parts.

If I'm not misreading the purpose of this issue, then #31971 could possibly help to solve this issue. That is, the template part post type could be responsible to store block-based menus, perhaps using "Nav" as a special "template part area," and the navigation block could then fetch them according to theme location?

@talldan
Copy link
Contributor Author

talldan commented Oct 1, 2021

Interesting point @carlomanf. I've not been working on FSE much, so I have a hard time grasping the complexities. It looks like your PR had positive feedback, but needs to be brought to attention again, it might be worth adding it as a discussion point on the core-editor meeting agenda. It definitely seems like something to address early in the WordPress release cycle to avoid what happened in 5.8.

Template parts as a generic storage format for blocks is an interesting discussion point as well. I think currently they're tied to the Template Part block? If the navigation block can also read and write to template parts, I imagine it'd need a way to filter out non-navigation block template parts, and the template part block would need the opposite. Given that, I was thinking that a separate CPT might be best, but I don't know if that would mean losing some functionality of template parts.

@mtias any advice on this?

@carlomanf
Copy link

If the navigation block can also read and write to template parts, I imagine it'd need a way to filter out non-navigation block template parts, and the template part block would need the opposite.

Although I haven't tested it out much, I understand that template parts have an "area" taxonomy that re-badges the block as "Header" or "Footer" or in this case it could be "Navigation" with the special ability to read and write menus (i.e. nav_menu_item) as well as template parts of that "area" term.

@mtias
Copy link
Member

mtias commented Oct 1, 2021

Template parts are fairly generic containers, to which we added a thin layer of semantics with "areas". (Areas are essentially block variations of template parts.) When I referred to storing a menu's inner content like template areas that's what I had in mind. Whether "navigation menu" fits exactly in that concept is slightly cloudy, but for practical purposes it seems like the best place to start since we have the utilities in place.

There are other benefits as well since template parts will be geared towards being styling context providers for theme.json, so menus can benefit from that.

The parts that could end up being a bit odd are related to how a navigation would generally be contained within a header or footer, so it's an area within an area. There's nothing wrong with that, and the composability of template parts allows it well, but it could also be slightly odd to reason about.

In any case, it's likely that even if it uses template parts as the container we might need some special handling to account for the specificities of navigation going forwards.

@spacedmonkey
Copy link
Member

Sorry, I am coming into this discussion late but wanted to give my thoughts here.

I am on the face of it, against adding a new custom post type and any new storage system of blocks. Mostly because I believe that existing systems could be used to store this data. There are 3 existing storage system that could be used here.

  1. Reusable blocks - wp_block post type.
    There are off course existing REST API and utils to use this post type for storage. But it required, we could add a flag either by adding some post meta or a term, to make a reusable block as a navigation reusable block, making them easier to surface in the UI.

  2. Template - wp_template post type.
    Similar to reusable blocks, existing apis etc. Again here we could add a flag to make it is a navigation template part. But as noted by @mtias , it has a other benefits, that other template parts have.

  3. Menus - Menu taxonomy term.
    As noted, the current menu data structure is. There is a menu, stored as a term with the taxonomy, menu and all menu items, are stored as a custom post type, with type menu item. For the best compatibility with existing plugins and themes, using the structure is preferred. However, I do understand there may some block data at the menu level, that we would need to store. Do consider, the menu is a term, a WP_Term object. A term as a field called, description this is a free text field. It is unused for menus. So we could use it to store menu block data ( as a html string). This means, if a user could see the menu in the list of options to assign to menu locations etc. So when a function like wp_nav_menu is called in the template, we get the term object, check if the description has block is it ( by calling has_blocks ). If the menu description has blocks, render the blocks, instead of menu items. Developers would have to opt into this feature, but what is discussed here, is 100% possible. I am willing to put together a POC, if anyone is interested.

To be clear here, I think option 3, is the best option here, for compatibility with existing functionality in core. I would love to explore the option more.

@draganescu
Copy link
Contributor

draganescu commented Oct 3, 2021

Indifferent of how this lands, making the navigation block behave like a template part, holding its contents in a CPT opens up the possibility to allow it to work with both kinds of menus, block and classic, provided there is an entity that handles the conversion to and from classic trees to block trees.

image

This way the block does not need to hold complicated rules and mappings, becomes agnostic to the kind of menu it's editing, the problem being delegated to the WordPress API to provide compatibility with classic data.

This would make the menus API endpoints more complex as they have to be able to receive block menu data and some metadata and based on the latter decide how to proceed:

  • convert to classic data and save to classic menus CPT
  • don't convert and save to block menu CPT

This architecture allows to easily reuse the block to edit classic menus both in classic contexts and in block contexts. It also makes it easy to reuse classic menus in block themes even with saving to classic format, only optionally providing an "import and convert" option.

For the block there should be no difference. It should interface with a menu entity through the same entities system in Gutenberg. The server side API for that entity would have the logic to determine both where to save and whether conversion is needed.

For simple menus made of trees of links this depends exclusively on some meta that classic menus have and block menus don't. It then becomes easy to upgrade seamlessly from classic to block menus when blocks other than links are sent through the entity to the API. It even becomes possible to retrieve and edit complex block menus in classic themes provided the theme indicates via opt in that it's ready to render a block menu, and not use a walker.

@spacedmonkey
Copy link
Member

This way the block does not need to hold complicated rules and mappings, becomes agnostic to the kind of menu it's editing, the problem being delegated to the WordPress API to provide compatibility with classic data.

Not sure I agree with this. What are you are basically sayings, that you want a REST API, that will take a data in whatever format and just magically know what to do with it. This feels like it getting to be very problematic and hard to build.

In my mind, the rest api, is designed to represent data, like menu item and to able to create/edit/delete. This to up the client connecting to it, be gutenberg or many other applications, like headless site, to know how to send/receive data for said object. It is not the job of a REST API to migrate or otherwise transform data. I could see the rest api being used for a one of migration of data, maybe, but even this is not really what the rest api is designed for.

It also does not make sense to do transformations of data in the REST API. The definitions of the blocks live in javascript. Handling/mangling data into the correct format, feels like it should live in the client, close to defintion of the block. Also, if the format of block data changes in V2 of the menu item block, both the javascript and php will need to changed, doubling the work / maintenance overhead..

@carlomanf
Copy link

Whatever approach is taken, I see these being the important aims:

  1. It should be possible to include menus into a post/page/template/etc, without permanently converting them to a serialised navigation block. This is especially important when a PHP theme is currently active and the same menu might be in use in another theme location.
  2. It should be possible to include a serialised navigation block into a post/page/template/etc without saving a reusable entity. Saving a reusable entity might be an option, but it should not be forced.
  3. Users should not be confused between reusable and non-reusable blocks, but nor should they be overloaded with too many different blocks for the same semantic purpose.

I am finding that a lot of the potential "solutions" discussed earlier only achieve some of the above at the expense of the other(s). For example, a one-time and permanent migration from menus to serialised blocks compromises on number 1. Re-defining the navigation block to fetch from an external entity compromises on number 2. An "invisible" reusable wrapper compromises on number 3 because users might edit the content while forgetting it's reusable, but at the same time it would be clunky to have two different blocks for reusable and non-reusable navigation.

@draganescu
Copy link
Contributor

It should be possible to include a serialised navigation block into a post/page/template/etc without saving a reusable entity.

What is the use of this?

@carlomanf
Copy link

It should be possible to include a serialised navigation block into a post/page/template/etc without saving a reusable entity.

What is the use of this?

A few answers:

  • It's the way the navigation block currently works. (Maybe ask the person who built it that way!)
  • You might want to set up a navigation block inside one template only, or inside one template part only.
  • Most blocks don't force you to make them reusable, so it would look odd for this one to behave that way. That would be a behaviour consistent with menus, but not with blocks.

@getdave
Copy link
Contributor

getdave commented Oct 4, 2021

The current Reusable blocks implementation has "convert to static" function so hopefully it's not a huge stretch to have this be a feature for the Nav block as well.

@talldan
Copy link
Contributor Author

talldan commented Oct 4, 2021

The current Reusable blocks implementation has "convert to static" function so hopefully it's not a huge stretch to have this be a feature for the Nav block as well.

I don't think this is something that needs to be solved straight away, I personally find it difficult to understand the idea of a parent block that can be both static and reusable at the same time. The reusable block's 'Convert to static' option is different because it clearly removes itself when choosing that option. Template parts have a 'detach' option and that is the same.

A few answers:

  • It's the way the navigation block currently works. (Maybe ask the person who built it that way!)
  • You might want to set up a navigation block inside one template only, or inside one template part only.
  • Most blocks don't force you to make them reusable, so it would look odd for this one to behave that way. That would be a behaviour consistent with menus, but not with blocks.

These are good things to flag, but I don't think these are blocking issues.

The first is just an artefact of how it was done, but we should be able to challenge and change that if needed.

The second is still possible with a reusable navigation block.

The third is perhaps the most challenging aspect. I think that these reusable concepts already exist makes it ok to try this out, a lot of the difficult aspects of these kinds of blocks are already being improved (better save flow, focus mode editing), so that may smooth out the path ahead.

@adamziel
Copy link
Contributor

adamziel commented Oct 4, 2021

This way the block does not need to hold complicated rules and mappings, becomes agnostic to the kind of menu it's editing, the problem being delegated to the WordPress API to provide compatibility with classic data.

@draganescu To me it boils down to having if( classic menu ) { do one thing } else { do another thing } somewhere. API endpoint is one place, but I think we could move that if to Javascript and have two separate endpoints just as well.

@adamziel
Copy link
Contributor

adamziel commented Oct 4, 2021

In my mind, the rest api, is designed to represent data, like menu item and to able to create/edit/delete. This to up the client connecting to it, be gutenberg or many other applications, like headless site, to know how to send/receive data for said object. It is not the job of a REST API to migrate or otherwise transform data.

@spacedmonkey There are many APIs out there that accept input encoded either as JSON or multipart form-encoded format. Some others will return different data formats depending on request's Content-type header, as in text/html or application/json. It's just a different representation of the same data. While accepting different inputs could mean overhead if/when the data format changes, supporting different outputs should be pretty straightforward and low-maintenance. That is, if we need it (see my previous comment).

@spacedmonkey
Copy link
Member

@adamziel My worry is, if the menu block change, so new attributes or attributes that are saved in a different format. A v2/v3... we have to support this in the REST API and maintain BC forever. I just feel like this is not the right place this in the REST API. The rest api, is not migration tool. There are places, where this might happen now, but in this case, I think can and should be avoided.

@talldan
Copy link
Contributor Author

talldan commented Oct 7, 2021

Here's a very very early attempt at exploring using a template part in the navigation block - #35418

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] Enhancement A suggestion for improvement.
Projects
None yet
7 participants