Add/navigation blocks post processing after migration from menu items #36950
Conversation
|
Size Change: +1.91 kB (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
adamziel
requested review from
spacedmonkey,
tellthemachines and
TimothyBJacobs
as code owners
|
Thanks a lot @adamziel to have been work on it 👍 and the lead. However, I already found a mistake in the sample code provided return {
name: "navigation-language-switcher",
clientId: block.clientId,
};We don't return a block here and it triggers an error later in the execution I simply replaced it by a call to if( block.name === "core/navigation-link" && block.attributes?.url === "#pll_switcher" ) {
return createBlock( 'polylang/navigation-language-switcher' );
}for example It works fine like that and now it's the language switcher block which appears as a child of the navigation block However the language switcher block attributes isn't correctly set because the language switcher menu item options are a custom post meta which isn't returned by default. I will take a look at the I will let you know |
|
@manooweb you're bringing up a good point here about the custom attributes. I think the menu migration would have to PHP-only to support that. I imagine that would involve a new endpoint that would run the migration code that's already there. By the way, @talldan mentioned removing the PHP filter that runs on theme switch in #36604 (comment), which adds urgency to this issue.
Good catch, thanks! I just updated that snippet |
@adamziel it seems to me that it isn't the context we're using. I'm going to search in that direction instead. We're used to use the REST API for the language for example and it seems not to be difficult to push our custom post meta here. |
|
On my side, I studied a little bit further espacially because we didn't get the Polylang language switcher menu item meta (_pll_menu_item). So I register the meta to be able to get it when the menu-items endpoint is called during the conversion process. <?php
add_action( 'rest_api_init', 'pll_register_switcher_menu_item_options_meta_rest_field' );
function pll_register_switcher_menu_item_options_meta_rest_field() {
$return = register_meta(
'post',
'_pll_menu_item',
array(
'object_subtype' => 'nav_menu_item',
'description' => __( 'Language switcher menu item options.', 'polylang-pro' ),
'single' => true,
'show_in_rest' => array(
'schema' => array(
'type' => 'object',
'additionalProperties' => array(
'type' => 'integer',
),
)
),
)
);
}and I got correctly the meta with the menu-items REST API call. {
"id": 585,
"title": {
"raw": "Langues",
"rendered": "Langues"
},
"status": "publish",
"url": "#pll_switcher",
"attr_title": "",
"description": "",
"type": "custom",
"type_label": "Liste des langues",
"object": "custom",
"object_id": 585,
"content": {
"raw": "",
"rendered": "",
"block_version": 0
},
"parent": 0,
"menu_order": 1,
"target": "",
"classes": [
""
],
"xfn": [
""
],
"invalid": false,
"meta": {
"_pll_menu_item": {
"hide_if_no_translation": 0,
"hide_current": 0,
"force_home": 0,
"show_flags": 1,
"show_names": 1,
"dropdown": 0
}
},
"menus": 66,
"_links": {
"self": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items/585"
}
],
"collection": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items"
}
],
"about": [
{
"href": "http://support.local/wp-json/wp/v2/types/nav_menu_item"
}
],
"wp:term": [
{
"taxonomy": "nav_menu",
"embeddable": true,
"href": "http://support.local/wp-json/wp/v2/menus?post=585"
}
],
"wp:action-publish": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items/585"
}
],
"wp:action-unfiltered-html": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items/585"
}
],
"wp:action-create-menus": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items/585"
}
],
"wp:action-assign-menus": [
{
"href": "http://support.local/wp-json/wp/v2/menu-items/585"
}
],
"curies": [
{
"name": "wp",
"href": "https://api.w.org/{rel}",
"templated": true
}
]
}
}
Then I adapted the code provided, to pass the meta to the block creation function mapBlockTree( blocks, menuItems, mapper ) {
return blocks.map(
block => (
{
...mapper( block, menuItems ),
innerBlocks: mapBlockTree( block.innerBlocks, menuItems, mapper )
}
)
);;
}
addFilter(
'navigation.menuItemsToBlocks',
'polylang/include-language-switcher',
( blocks, menuItems ) => (
{
...blocks,
innerBlocks: mapBlockTree(
blocks.innerBlocks,
menuItems,
( block, menuItems ) => {
if( block.name === "core/navigation-link" && block.attributes?.url === "#pll_switcher" ) {
const menuItem = find( menuItems, { url: '#pll_switcher' } ); // Get the corresponding menu item.
const attributes = menuItem.meta._pll_menu_item; // Get its options.
return createBlock( navigationLanguageSwitcherName, attributes );
}
return block;
}
)
}
)
);However, even if the menu item got the correct meta during the process The block attributes UI didn't display the right options.
I noticed that the
So I thought because the navigation block mapping wasn't updated. Then I adapted the code again to take the blocks mapping with the new language switcher block client id. function mapBlockTree( blocks, menuItems, blocksMapping, mapper ) {
return blocks.map(
block => (
{
...mapper( block, menuItems, blocksMapping ),
innerBlocks: mapBlockTree( block.innerBlocks, menuItems, blocksMapping, mapper )
}
)
);;
}
addFilter(
'navigation.menuItemsToBlocks',
'polylang/include-language-switcher',
( blocks, menuItems ) => (
{
...blocks,
innerBlocks: mapBlockTree(
blocks.innerBlocks,
menuItems,
blocks.mapping,
( block, menuItems, blocksMapping ) => {
if( block.name === "core/navigation-link" && block.attributes?.url === "#pll_switcher" ) {
const menuItem = find( menuItems, { url: '#pll_switcher' } ); // Get the corresponding menu item.
const attributes = menuItem.meta._pll_menu_item; // Get its options.
const newBlock = createBlock( navigationLanguageSwitcherName, attributes );
blocksMapping[ menuItem.id ] = newBlock.clientId;
return newBlock;
}
return block;
}
)
}
)
);Unfortunately it changed nothing to the result the language switcher block attributes aren't right in the UI. I don't see how to figure out this issue and where to search. If you have an idea I'm very interested in Many thanks |
|
@manooweb somehow I can't find the Anyway, let me take my best guess: const attributes = menuItem.meta._pll_menu_item; // Get its options.
const newBlock = createBlock( navigationLanguageSwitcherName, attributes );And so the call unwinds to const newBlock = createBlock(
"polylang/navigation-language-switcher",
{
dropdown: 0,
force_home: 0,
hide_current: 0,
hide_if_no_translation: 0,
show_flags: 1,
show_names: 1,
}
);When I think you're after: const newBlock = createBlock(
"polylang/navigation-language-switcher",
{
dropdown: 0,
forceHome: 0,
hideCurrent: 0,
hideIfNoTranslation: 0,
showFlags: 1,
showNames: 1,
}
);LMK if that helped. |
I'm aware it's difficult to understand without this part of the code but it is developed in the premium version only.
I don't think it's a case problem with the attribute names. The block itself works correctly with these attributes in snake case.
We develop like that to be consistent with the language switcher in PHP code espacially to maintain these options only in one place. |
|
@adamziel I extracted in a GH repository the code necessary to have the language switcher block as a stand alone block or as a child of the You have a readme file to explain how to install it and how to use it with the second use case that we are interested in here. https://github.com/manooweb/gutenberg-block-navigation-conversion-test If you need any other explanation or have issues, LMK 😉 Thanks |
|
@manooweb thanks! I identified the problem. The attributes are registered as |
@adamziel Yes!! That helps 👍 Thanks |
|
@adamziel LGTM And now the result is as expected From the classic menu, I checked and the conversion to the corresponding block in the Edit: in fact it depends on how to declare REST API schema to retrieve the meta. I had declared So I reverted the JS code and I corrected the Do you think this PR can be merged in the next release and available in the next WordPress release? Many thanks for the work and time to help 👍😉 |
…-migration-from-menu-items
@manooweb I believe so! I just need an approval here :-) @getdave @noisysocks @talldan @draganescu @Mamaduka anyone willing to approve ? |
|
I think this is a smart solution to the problem but I have a few, potentially convoluted, concerns: Will we ever want to transform classic menus into something else other than submenu and link? Adding this filter will be a contract that we always return submenu and link to extenders.
This is a 1st in the block library :)
Looking at the problem:
All my concerns are summed up by just being worried that by adding a filter on client side conversion we make a contract about the structure of the navigation block to the world. |
|
I will defer to @adamziel who no doubt is better placed to answer than I. Nonetheless:
I don't think we are forcing users to return a particular structure. We're giving them the option to hook into each menn item and change how that is transformed into a block. If they want menu item X to turn into multiple blocks equivalents then that's their choice. They could just as easily do nothing or just subtly change the conversion. Indeed Adam's example shows you can just
We are making a contract on that filter. I felt it was safe to assume that we will always want to provide a means of transforming menu items to blocks. If we're not 100% sure that's not going to be a feature then perhaps we need to make this filter experimental for a while and make a note to promote to first class only after a few Gutenberg Plugin versions have elapsed. |
|
@draganescu, today we can extend "classic" menu with custom menu item and the current version of We need to change the conversion behaviour to catch the custom menu item and change its conversion to another block than the default |
|
@manooweb Yes, I understand the need and I do think this conversion should be changeable by extenders. My worry is about this solution because it is unique in the block library and because it may impact future updates to the block. |
Thanks for bringing it up @draganescu. I agree, I even wanted to explore backend-only conversion to blocks for 6.0. The usual way of navigating the uncertainty is to call things |
adamziel
added
the
Backport to WP 6.7 Beta/RC
There was a problem hiding this comment.
With an unstable label this can move forward. I think we'll be better off in the future to remove client side filtering and instead allow extenders to append detail to the output of the menu items endpoint which instructs the conversion, or do the conversion itself serverside.
noisysocks
deleted the
add/navigation-blocks-post-processing-after-migration-from-menu-items
branch
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
…#36950) * add navigation.menuItemsToBlocks filter to postprocess blocks created from menu items * Add navigation_after_parse_blocks_from_menu_items filter * Make the filter experimental * Lint * Update menu-items-to-blocks.js
|
@draganescu Yes! I understand too and prepare our release with the new unstable filter name. Thanks all for the reactivity and by backported this in the beta/RC releases 👍 |
|
Is this ticket not being backported? |
adamziel
added
Backport to WP 6.7 Beta/RC
|
I think it is here WordPress/wordpress-develop#2048 |
Description
Related to #36616
Problem:
Proposed solution:
Add a set od filters to postprocess the
menu items -> blocksconversion results. A usage example for the Polylang plugin would look like this:Sample usage in JS (runs after selecting a classic menu in the block):
A corresponding filter would have to be implemented in PHP to catch the conversion that runs when switching to a block theme.
As a side note, the migration could potentially be handled entirely on the PHP side instead of being implemented in both PHP and JS – but that's for another day. cc @noisysocks @talldan @getdave @manooweb
How has this been tested?
Confirm that all the checks are green.