Navigation Area block #36178
Navigation Area block #36178
Conversation
adamziel
requested review from
ajitbohra,
nerrad,
spacedmonkey,
tellthemachines and
TimothyBJacobs
as code owners
adamziel
added
[Block] Navigation
adamziel
force-pushed
the
try/nav-area-block
branch
from
bb761a5 to
f353cae
Compare
|
Size Change: +488 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Something like this could be useful in the if ( ! empty( $block->context['navigationArea'] ) ) {
$area = $block->context['navigationArea'];
$mapping = get_option( 'fse_navigation_areas', array() );
if ( ! empty( $mapping[ $area ] ) ) {
$attributes[ 'navigationMenuId' ] = $mapping[ $area ];
}
}Edit: I just added it |
|
In testing, it's working very well. Switching themes is working flawlessly when they have these navigation areas defined. I don't see any reason not to progress with this. It'd be a matter of priority to update bundled theme templates with this new block and to get the word out to theme developers about it (I'll add 'Needs dev note'). There's a small bit of a glitch when switching menus sometimes—on the odd occasion it considers the menu 'unsaved'. I think this is a bug in Some potential opportunities:
|
talldan
added
Needs Dev Note
There was a problem hiding this comment.
Tentatively giving this approval, so that there's no issue with it being merged in time for 5.9.
Really happy with how it works, and it looks like there are only minor changes and tests remaining. Look forward to getting this shipped, great work @adamziel!
adamziel
force-pushed
the
try/nav-area-block
branch
from
d535b49 to
18fe270
Compare
talldan
force-pushed
the
try/nav-area-block
branch
from
ad86149 to
e7bc4a1
Compare
|
Merging for good luck. |
Yes please! Not just for Universal themes but for users who switch from a classic theme to a block theme too |
| navigationAreas?.map( ( { name, description } ) => ( { | ||
| label: description, | ||
| value: name, | ||
| } ) ), |
There was a problem hiding this comment.
Let's do || [] to prevent a null value making it to MenuItemsChoice
| ); | ||
| return { | ||
| navigationAreas: getEntityRecords( 'root', 'navigationArea' ), | ||
| hasResolvedNavigationAreas: hasFinishedResolution( |
There was a problem hiding this comment.
We could do && navigationAreas.length to prevent pretending everything is fine when there are no areas
👍 Absolutely, that's a very good point, we need to get our thinking caps on about the classic theme to block theme migration. It shouldn't be too difficult, we need hook into theme switch, figure out which classic menu is assigned to a corresponding navigation area, create a wp_navigation post with the menu items converted to blocks, and then assign its id to the navigation area. |
I detailed the existing classic mechanic in #35750 (comment). Hopefully this serves as inspiration for an automated mapping solution. I will say again that I think that on Theme switch, the classic menu should be copied/cloned and converted into a block based menu leaving the original in place. If the user switches back to a classic theme, then we should allow that classic theme to simply pull through the original classic menu. |
|
Maybe the first, |
|
Linking this here: Migrate classic menus to block-based menus on theme switch |
| 'callback' => array( $this, 'update_item' ), | ||
| 'permission_callback' => array( $this, 'update_item_permissions_check' ), | ||
| 'args' => $this->get_endpoint_args_for_item_schema( WP_REST_Server::EDITABLE ), | ||
| ), |
There was a problem hiding this comment.
Are we going to add support for batching?
| foreach ( gutenberg_get_navigation_areas() as $name => $description ) { | ||
| $area = $this->get_navigation_area_object( $name ); | ||
| $area = $this->prepare_item_for_response( $area, $request ); | ||
| $data[ $name ] = $this->prepare_response_for_collection( $area ); |
There was a problem hiding this comment.
@TimothyBJacobs what do you think if this? Other endpoints simple are a flat array. A key based array is different from other endpoints.
There was a problem hiding this comment.
We do have a couple of object responses, but an array is preferable because the REST API does assume that collection responses are lists.
| private function get_navigation_area_object( $name ) { | ||
| $available_areas = gutenberg_get_navigation_areas(); | ||
| $mapping = get_option( 'fse_navigation_areas', array() ); | ||
| $area = new stdClass(); |
There was a problem hiding this comment.
Could we create a custom object for this?
| ); | ||
| foreach ( $active_areas as $post_id ) { | ||
| if ( 0 !== $post_id ) { | ||
| $paths[] = "/wp/v2/navigation/$post_id?context=edit"; |
There was a problem hiding this comment.
$paths[] = add_query_args( 'context', 'edit', rest_get_route_for_post( $post_id ) );
|
Sorry I missed this PR. I added my thoughts on the PR anyway, in hope that some of my questions are answered |
| function gutenberg_register_navigation_areas( $new_areas ) { | ||
| global $gutenberg_navigation_areas; | ||
| $gutenberg_navigation_areas = $new_areas; | ||
| } | ||
|
|
||
| // Register the default navigation areas. | ||
| gutenberg_register_navigation_areas( | ||
| array( | ||
| 'primary' => 'Primary', | ||
| 'secondary' => 'Secondary', | ||
| 'tertiary' => 'Tertiary', | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Something I just noticed. If a theme dev is able to change the keys here by calling register_navigation_areas it'll degrade the capability of theme migration. This is a problem we have with the existing menus system in WordPress.
An example - one theme calls a menu location 'Header', the other 'Primary', and when switching no migration happens because the names don't match.
The way around this, I think, is to keep an internal representation of the area primary, secondary (or maybe even just an index 0, 1), but allow users to set a name that's used in the UI ('Header', 'Footer').
I quite like the idea of:
array(
'primary' => 'Header',
'secondary' => 'Sidebar',
'tertiary' => 'Footer',
)But then what if I have four or five or ten navigation areas? What comes after tertiary?
It's a difficult problem, but I think we should try to solve it for 5.9 by improving this function. Lets come up with some ideas for this.
There was a problem hiding this comment.
I think we should provide some strong defaults but allow users to change extend how they like.
Most devs will stick with the convention that comes by default so that will solve 80% use case of Theme switch.
If a Theme dev choose to alter the area name then they will need to handle the situation where navigations don't migrate across on Theme switch. This provides a big incentive to stick with the registered defaults.
I didn't realise that register_navigation_areas would overwrite the existing items. If that's the case then I think we should make that function private and instead provide a filter. This approach is more "WordPress" and also tends to encourage extension rather than wholesale overwriting.
What comes after tertiary?
quaternary, quinary, senary, septenary, octonary, nonary, denary.
There was a problem hiding this comment.
I've added a comment over here - #36373 (comment)
talldan
removed
the
Needs Dev Note
Description
This PR proposes preserving navigation on theme switch by introducing a "Navigation Area" block.
Relies on #36212
The gist here is that:
{ "primary": 794, "secondary": 112 }Theme authors would ship the following HTML in their templates:
Which makes the nested navigation block use the
wp_navigationpost associated with theprimaryarea.Changing saving the navigation area changes the post ID to which the primary area is mapped. The nice part is that this PR uses the entity system, which gives us things like in-editor synchronization and confirmation dialogs for free.
See this a short video from the site editor. I wrestled with GB a bit but it worked more-or-less as intended, maybe except of the unsaved menu:
CleanShot.2021-11-03.at.15.59.49.mp4
Switching to another theme just works.
About design choices
setAreaMenuif another menu is selected.What's missing: