-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Don't try and render unstable location if Nav block has ID #36863
Don't try and render unstable location if Nav block has ID #36863
Conversation
Looks good to me, but cc-ing @talldan who might have more context here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solves the problem for me.
…as long as there are items at that location.
I added a commit to address #36865 So the priority is:
|
02079cc
to
c6fc6da
Compare
array_key_exists( '__unstableLocation', $attributes ) && | ||
! array_key_exists( 'navigationMenuId', $attributes ) && | ||
! empty( gutenberg_get_menu_items_at_location( $attributes['__unstableLocation'] ) ) | ||
) { | ||
$menu_items = gutenberg_get_menu_items_at_location( $attributes['__unstableLocation'] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is now an inefficiency here as we are calling gutenberg_get_menu_items_at_location
twice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow up here.
* Don't try and render unstable location if Nav block has ID * Reinstate condition for empty inner blocks * Lint * Show the unstableLocation menu items even if there are inner blocks, as long as there are items at that location. Co-authored-by: Adam Zieliński <adam@adamziel.com> Co-authored-by: Ben Dwyer <ben@scruffian.com>
Description
We learnt in Automattic/themes#5086 that if you are using a Universal Theme the Nav block can end up rendering nothing even though there are items added in the Site Editor.
Since the Navigation block now stores its inner "items" in a CPT
there a no longer an inner blocks stored directly on the block itself.the only time when inner blocks are stored directly on the block is if the block is part of a pattern. If you are working with the Nav block within the editor your changes are saved to the CPT and not as inner blocks.If
__unstableLocation
is defined (universal Themes) then even if you add "items" to the Nav block in the Site Editor, the front end rendering code will try and render the menu at__unstableLocation
. If such a Menu does not exist then nothing is shown. This is due to the conditional:gutenberg/packages/block-library/src/navigation/index.php
Line 259 in f439aef
This is wrong. The contents of the Site Editor should take precedence. The reason they aren't is that the code is expecting the contents of the Nav block to be the inner blocks whereas this can now also be indicated by the presence of a
navigationMenuId
attribute (which references thewp_navigation
CPT where the nav "items" are stored).Addresses Automattic/themes#5086
Closes #36865
How has this been tested?
On trunk
On this PR
Screenshots
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).