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

Add/dynamic navigation block #16796

Merged
merged 21 commits into from
Aug 29, 2019
Merged

Add/dynamic navigation block #16796

merged 21 commits into from
Aug 29, 2019

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented Jul 29, 2019

Description

Closes: #16810

In order for the navigation block to remain up to date on the front end it needs to be dynamic, rendered on the server.

In this PR we introduce the following updates to the Navigation Block:

a) a new shim is added in compat.php which passes the block structure to the block render callback on the server. This is useful because otherwise we don't have access to the innerBlocks structure from the current args, attrs and content. With the shim the render callback receives a block argument which is a full block data structure, including its inner blocks.

b) the Navigation block is registered and rendered serverside.

c) the UI is updates so that we can test infinite block nesting, and the appender is replaced with the default button appender. If you test, the experience is clumsy but it works.

@noisysocks
Copy link
Member

@draganescu: I rebased this branch and pushed up 5b2cf72 which shows one way we could access inner blocks from within render_callback. Basically, we modify Core so that it passes the full $block object to render_callback. The Gutenberg plugin can shim this functionality until it exists in WordPress 5.3.

@draganescu
Copy link
Contributor Author

This is great @noisysocks. I think we need to customize more the way the blocks tree is represented if we need / want to use the Walker class. This tree walker is very simple and it requires that each item have an ID and a parent "field". Currently the tree of blocks provided by gutenberg_provide_render_callback_with_block_object does not allow a walker to know from a leaf which block (item) is that leaf's parent.

However I am unsure of where should we update this structure? Should gutenberg_provide_render_callback_with_block_object change the way the structure looks or should the block itself provide another "decorator" for that structure and then pass that to the Walker?

Anyway, either way, we're on the right track I think and this solution is quite handy in providint the render callback with a nested block structure.

@draganescu
Copy link
Contributor Author

@noisysocks there is a barebones sample walker illustrating how to convert the tree returned by parsing blocks into a structure that is handled well by the WP walker.

The goal is to have wp_nav_menu work with navigation blocks menu as well, and for that a custom walker isn't really enough, we also need to update wp_nav_menu itself, I think.

@noisysocks
Copy link
Member

However I am unsure of where should we update this structure? Should gutenberg_provide_render_callback_with_block_object change the way the structure looks or should the block itself provide another "decorator" for that structure and then pass that to the Walker?

I'd say the latter. render_callback should just be given a $block plain and simple. (I'm undecided as to whether $block should be an array or a WP_Block, mind you—we can evaluate this later.)

@draganescu draganescu force-pushed the add/dynamic-navigation-block branch from 02f7f53 to c0693eb Compare August 22, 2019 13:39
@draganescu draganescu marked this pull request as ready for review August 26, 2019 07:02
@draganescu draganescu force-pushed the add/dynamic-navigation-block branch from 73cf37a to 74c1a41 Compare August 26, 2019 07:20
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking close! Thanks for working on it @draganescu.

I left a few (mostly minor) comments.

I checked this out locally and played with it a little. The UI is obviously very rough, but the foundations are there. I'm happy to get this in so that we can start work on building out a nice UI for manipulating Menu and Menu Item blocks.

Something I'd like to address before merging is that the markup which appears in the Code Editor (Cmd+Opt+Shift+M) does not match the markup that ends up on the site's frontend.

For example in the Code Editor I see:

<!-- wp:navigation-menu -->
<nav class="wp-block-navigation-menu"><!-- wp:navigation-menu-item {"label":"Amsterdam"} -->
<a>Amsterdam</a>
<!-- /wp:navigation-menu-item -->

<!-- wp:navigation-menu-item {"label":"Bangladesh"} -->
<a>Bangladesh</a><!-- wp:navigation-menu-item {"label":"Brussels"} -->
<a>Brussels</a>
<!-- /wp:navigation-menu-item -->
<!-- /wp:navigation-menu-item -->

<!-- wp:navigation-menu-item {"label":"Charlotte"} -->
<a>Charlotte</a>
<!-- /wp:navigation-menu-item --></nav>
<!-- /wp:navigation-menu -->

And on the frontend I see:

<ul><li>
<a>Amsterdam</a>
</li><li>
<a>Bangladesh</a><ul><li>
<a>Brussels</a>
</li></ul></li><li>
<a>Charlotte</a>
</li></ul>

The Code Editor markup is missing <ul>s and <li>s, and the frontend markup is missing <nav class="wp-block-navigation-menu">.

Some clients (RSS readers, etc.) are only able to see what we save to post_content and not the dynamic block output, so it's important that the markup that we return from save() is near enough to the markup that's on the frontend.

/** This filter is documented in src/wp-includes/blocks.php */
return apply_filters( 'render_block', $block_content, $block );
}
add_filter( 'pre_render_block', 'gutenberg_provide_render_callback_with_block_object', 10, 2 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once merged, let's remember to create a Trac ticket to update WP Core with these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create the ticket now, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep go for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a ticket already for that? Should we land it in 5.3 to avoid forgetting about it later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
@draganescu
Copy link
Contributor Author

I have the save functions of both the navigation menu and navigation menu item return only <InnerBlocks.Content />, not null since we need this saved so it gets passed to the server side render function.

@noisysocks let's see if there's anything to do here.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd like to see us store some of the navigation menu's HTML in post_content, and then enhance it in render_callback, but this is a great solution for now. Could you please create an issue to remind us to look into this? #17231 tracks looking into this.

Thanks for working on this @draganescu! Let's get it in and iterate! 😀

packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
packages/block-library/src/navigation-menu/index.php Outdated Show resolved Hide resolved
*/
function build_navigation_menu_html( $block ) {
$html = '';
foreach ( (array) $block['innerBlocks'] as $key => $menu_item ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that $menu_item has the expected type of core/navigation-menu-item? (We can do this in a follow-up PR.)

@noisysocks noisysocks merged commit 4f052a1 into master Aug 29, 2019
@noisysocks noisysocks deleted the add/dynamic-navigation-block branch August 29, 2019 00:56
@gziolo gziolo added this to the Gutenberg 6.5 milestone Aug 30, 2019
@gziolo gziolo added the [Block] Navigation Affects the Navigation Block label Aug 30, 2019
@eklingen88
Copy link

I was just pulling my hair out trying to figure out why my default attributes weren't showing up. Digging back through the callstack I found:

gutenberg/lib/compat.php

Lines 97 to 98 in 5774046

$block_type->prepare_attributes_for_render( $block['attrs'] );
$block_content = (string) call_user_func( $block_type->render_callback, $block['attrs'], $block_content, $block );

The attributes are properly prepared, but not sent to the render_callback function.

I believe that it should be something more like this:

wp-includes/class-wp-block-type.php

		$attributes = $this->prepare_attributes_for_render( $attributes );

		return (string) call_user_func( $this->render_callback, $attributes, $content );

@epiqueras
Copy link
Contributor

The attributes are properly prepared, but not sent to the render_callback function.

Yes, I believe this breaks default attribute replacement in server side rendering. This needs to be fixed before the next release, or back-ported if already released.

cc @noisysocks @draganescu @jorgefilipecosta

@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Sep 16, 2019
@obenland obenland mentioned this pull request Oct 8, 2019
5 tasks
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 4, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Block: Make it Dynamic
7 participants