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

Navigation: Make the block-nav-menus setting more granular #24638

Closed
Tracked by #29102
noisysocks opened this issue Aug 19, 2020 · 5 comments
Closed
Tracked by #29102

Navigation: Make the block-nav-menus setting more granular #24638

noisysocks opened this issue Aug 19, 2020 · 5 comments

Comments

@noisysocks
Copy link
Member

As of #24503, themes are able to indicate that they support having non-link blocks in existing menu areas by adding add_theme_support( 'block-nav-menus' ).

However, it may be the case that themes want to use blocks in one menu but not another. We should look at making the block-nav-menus setting more granular.

One approach might be to make it a per menu location setting instead of a per theme setting by updating register_nav_menu to accept a supports argument.

register_nav_menu(
	'primary',
	array(
		'description' => __( 'Desktop Horizontal Menu' ),
		'supports'    => array(
			'blocks' => true,
		),
	)
);

See #24503 (comment).

@manmotive
Copy link

manmotive commented Aug 19, 2020

Thanks for creating this issue.

My initial thoughts are that it's not quite granular enough. There are many themes out there that use the same menu location in different positions on the same per page, but style it differently. A commonish use case is a theme that registers a single primary menu location. On desktop it displays as a standard horizontal menu (which might support blocks), and on mobile it displays a mobile menu (which might not support blocks - especially in the case of themes that use a mobile menu script like https://mmenujs.com/). Due to that, I'd suggest this is something that's enabled/disabled at wp_nav_menu level, it seems to fit nicely with the other wp_nav_menu options and could be switched on/off with filters (I don't think that's possible with register_nav_menu, but I might be wrong?).

@manmotive
Copy link

manmotive commented Aug 21, 2020

I thought I'd add some examples which I think should be possible by making this a wp_nav_menu option.

For existing calls to wp_nav_menu, there would be no change:

wp_nav_menu( array( 
    'theme_location' => 'primary'
) );

For existing calls to wp_nav_menu with a custom walker, there would be no change:

wp_nav_menu( array( 
    'theme_location' => 'primary',
    'walker' => new WP_Bootstrap_Navwalker()
) );

To switch from walker output to block output, the wp_nav_menu call would be updated:

wp_nav_menu( array( 
    'theme_location' => 'primary'.
    'supports_blocks' => true
) );

.. unless a custom walker is also specified, in which case the walker with blocks would be used (i.e. the proposed bit of code which strips blocks from wp_nav_menu_objects would be skipped in this case):

wp_nav_menu( array( 
    'theme_location' => 'primary',
    'walker' => new WP_Bootstrap_Navwalker(),
    'supports_blocks' => true
) );

Edit: or maybe the walker itself could have a property that says it's block-ready 🤔

I think that covers all possible use cases with only one extra option for wp_nav_menu().

The existing wp_nav_menu_args filter could be used to switch these options on and off (e.g. in the case where a theme author has switched block support on, but a particular user wants to switch it back off).

For me, from a selfish point of view, I'd like to be able to bring block support to my menu walker system so I'm getting in early with that request 😀 (I'm sure I won't be the only one!)

I can see the reasoning behind adding the option to register_nav_menu, it does seem nice and "neat", but conventionally it is the wp_nav_menu call that determines the output method of menu (through the walker argument).

@noisysocks
Copy link
Member Author

I think you're right about adding an option to filter out blocks to wp_nav_menu. The problem with my proposal to add a setting to the menu location is that the Navigation screen wouldn't be able to tell whether or not to allow blocks in a menu until the user associates that menu with a menu location.

For me, from a selfish point of view, I'd like to be able to bring block support to my menu walker system so I'm getting in early with that request 😀 (I'm sure I won't be the only one!)

Could you go into your use case here? I don't really understand why it would be useful to customise how WordPress renders a tree of blocks into HTML. There are probably more "block friendly" ways of doing this using child blocks and dynamic blocks.

@manmotive
Copy link

the Navigation screen wouldn't be able to tell whether or not to allow blocks in a menu until the user associates that menu with a menu location.

A menu might also tagged to 2 locations: one that supports blocks, one that doesn't.

Could you go into your use case here? I don't really understand why it would be useful to customise how WordPress renders a tree of blocks into HTML. There are probably more "block friendly" ways of doing this using child blocks and dynamic blocks.

Let me rewind a bit, I'm not sure if we're on the same page :) My understanding with the current proposal is: as soon as "blocks" are enabled for a menu location, instead of a walker being used, the menu is rendered as a navigation block. I'd like the option, somehow, to also be able to output blocks using a walker (as in the initial proposal for blocks in menus). I think you may have just covered that with:

I think you're right about adding an option to filter out blocks to wp_nav_menu.

@Thelmachido
Copy link

Closing this issue due to the Navigation Editor project being moved to an inactive status on the feature projects page in coordination with the project leads. If this work is picked back up, issues can be revisited and reopened as needed.

At the moment we have the navigation block, the work can continue there

@Thelmachido Thelmachido closed this as not planned Won't fix, can't repro, duplicate, stale Sep 20, 2022
Navigation editor automation moved this from Needs dev to Done Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants