Navigation block: Add an unstable location attribute #32491
Conversation
draganescu
left a comment
draganescu
left a comment
There was a problem hiding this comment.
This PR is pretty good like this. Left two nitpicks.
| * @param array $attributes Navigation block attributes. | ||
| * @return string HTML markup of a generated Navigation Block. | ||
| */ | ||
| function get_classic_navigation_elements( $attributes ) { |
There was a problem hiding this comment.
We could rename get_classic_navigation_elements to render_classic_location_menu because that is what the function does, and the getter aspect is not correct. I would also change the signature to include $location as a parameter since it's a bit opaque otherwise => render_classic_navigation( $location, $attributes ) : mixed {}; I'd make it return false for an empty location.
|
|
||
| if ( empty( $block->inner_blocks ) ) { | ||
| return ''; | ||
| return get_classic_navigation_elements( $attributes ); |
There was a problem hiding this comment.
This code reads "if the block is empty render a classic menu", which is not correct. It should read "if the block is empty and there is an associated classic menu, render that". So, what about this as an option:
- rename
get_classic_navigation_elementstorender_classic_location_menu - update
render_classic_location_menuto return false if no location is set - change this to:
if ( empty( $block->inner_blocks ) ) {
$maybe_classic_navigation = render_classic_location_menu( $attributes );
if ( $maybe_classic_navigation ) {
return $maybe_classic_navigation;
}
return '';
}
scruffian
force-pushed
the
add/unstable-location-to-navigation
branch
from
566b7c3 to
de22b21
Compare
draganescu
left a comment
draganescu
left a comment
There was a problem hiding this comment.
I believe this is a minimal change and it's worth merging like this. We'll see how the location attribute evolves. Also giving the block the power to render a classic menu is a superpower useful for theme migration. 🚢 it.
|
Was the main problem of #30852 addressed (that it relies on |
| "type": "boolean", | ||
| "default": false | ||
| }, | ||
| "__unstable__location": { |
There was a problem hiding this comment.
It should be __unstableLocation. Attributes are camel case.
There was a problem hiding this comment.
🤦🏻♂️ that's my bad. Will update.
Yes, we're not checking on that anymore, if the location attribute is present we render the menu found in said location using |
| return wp_nav_menu( | ||
| array( | ||
| 'theme_location' => $location, | ||
| 'block_attributes' => $block_attributes, |
There was a problem hiding this comment.
This is not needed I think.
|
Great, thank you for addressing that. 👍 The
I don't think that's the case now, so maybe that could be updated too. |
| * @param array $attributes Navigation block attributes. | ||
| * @return string|false HTML markup of a generated Navigation Block or false if no location is specified. | ||
| */ | ||
| function render_classic_location_menu( $location, $attributes ) { |
There was a problem hiding this comment.
Can we rename this to render_menu_from_location or something to that effect?
There was a problem hiding this comment.
Ah, it should probably be prefixed with gutenberg_ too.
Description
This is a simplified version of #30852.
All it does is add an
__unstable__locationattribute to the block and then, if that is set, render a classic menu in its place.This opens some questions about if/how this should be handled in the UI but these can be dealt with in followup PRs if necessary.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).