Register Navigation Link block variations based on menu item types added in customizer filters #31584
Conversation
Aljullu
added
[Feature] Extensibility
Aljullu
requested review from
adamziel,
ajitbohra,
draganescu,
ellatrix,
kevin940726,
nerrad,
noisysocks,
spacedmonkey,
talldan,
tellthemachines and
TimothyBJacobs
as code owners
|
Size Change: +70 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
Aljullu
force-pushed
the
fix/31316-honor-customize-filters-navigation
branch
2 times, most recently
from
940e8d0
to
c79bae3
Compare
|
Thank you for raising this PR. It looks really interesting and I'm pleased that we have an approach to catering for these existing filters. Obviously, this is quite a large PR so ultimately (not yet - let's review first) we might want to break it up into PRs for:
That should make it easier to review and (given the pace of development in Gutenberg at the moment) introduce without too many rebases along the way. I'm going to experiment with the branch as per your instructions and start to wrap my head around what's going on and whether there are any other angles to explore. Initially, however, this seems to fit together well. |
|
Hello, @Aljullu Thanks for working on this. I agree with @getdave that splitting this into smaller PR will make reviews easier. One thing that I'm not certain about is if we should use Maybe we should introduce new navigation screen-specific filters for adding custom variations and fetching suggestions. |
|
Thanks for the answers @getdave and @Mamaduka!
From my understanding, there were two ways plugins were adding custom menu items (at least, that's what WooCommerce is doing):
The issue with the second approach is that it needs to add a lot of boilerplate HTML to create the meta box layout, while the first approach seems cleaner. It's true, however, that the filter names might not be the most appropriate for the block-based navigation screen. I used those filter names to keep it backwards-compatible, but we could rename them and deprecate (but still support) the old filter names. Is that what you were proposing?
Will do! 🙂 I just want to make sure we agree on a path forward before opening new PRs for this. |
Yes, the current method for adding custom menu items to the menu screen isn't ideal. Maybe we can add This will make it easier for plugins to opt-in new Navigation screen support by adding another filter for the same callback. |
|
Before we chat through implementation details, I want to better understand the problem this is solving. So broadly, would we say we'd like for a site to define some set of links which are insertable in the navigation block? Are these links dynamic? How are they special/different from custom links and do we have any other examples besides WooCommerce? |
Yes, that's correct. These links can be dynamic or not. Most of the time, they're inserted as "Custom Link" type. I know that BuddyPress also uses this method, and based on my quick search of the WP.org repo, there are 20 other plugins. Search results: https://wpdirectory.net/search/01F61B3YJ3XNDYPJS2PCYW8HEZ |
@Mamaduka That sounds good to me. The idea would be that the Navigation screen would only apply these new filters and extensions would need to register filters for the legacy and new hooks?
@gwwar Those links are not dynamic, once added they are just regular 'custom links', at least, this is the case for WooCommerce. The utility of those filters is so plugins can offer a set of links that users can choose from without the need to introduce the URL (which the user might not know at all). |
|
@Aljullu I've tried several times to get a WooCommerce build up and running but unfortunately I keep hitting memory limits. For ease of testing and for the benefit of other reviewers, would you be able to add some code directly to this PR (or in a comment) that utilises the functionality being added by this PR? It really just needs to exercise the new APIs you've created so we can review. I feel that's going to make the process of getting feedback here a lot easier. Let us know if you're able to do that. Much appreciated. |
|
@getdave done! 👍 I updated the PR description with a PHP snippet that can be used to test this PR, let me know if it works. |
There was a problem hiding this comment.
Thanks for looking into this @Aljullu. I'm leaving a bit of higher level feedback to 🤔 through.
Overall, I do think this is a pretty good use case to support, but I'm not sure of how this should be displayed to a user in the block editors.
How many custom links does a plugin typically register? Is it something like a handful like 5 or a lot like 1000+? If it's closer to a handful we could maybe embed this within a script or the block variation data itself. If it's a lot, some REST endpoint makes more sense. (Whether this is new or an extended extended endpoint I'll defer to API component owners).
Another thing that could use some thought is discoverability of said custom links. By default, link suggestions only displays 3 links. How will a user know to search for items they didn't create? Maybe @jasmussen has some ideas?
custom.links.mp4
|
This is definitely tricky from a UI point of view. As Kerry points out, the more child blocks we add, the harder it is to find the right one to insert. It's already grown to a point where the experience isn't ideal:
The fact that it's a two-step process: insert block type first, and then choose the contents of that block type is likely a source of confusion for a user who inserts a navigation block and then expects the plus button to let them immediately add a link. But that's how the block editor works — first you insert the block, and then you configure the block. Patterns is a key interface we can use to smooth this out. And that brings me to the next item — navigation block patterns will likely have menu items, search, spacers, site titles, social links preinserted. They will not pre-insert a block type that isn't part of the vanilla offering. Which means any blocks you extend with here, have to be inserted by the user as they are building their menu — they won't be able to leverage patterns. Even now, we have both "Page Link", and "Custom Link" blocks to insert. They are technically the same, mostly — you can search for a Page in the custom Link block, or you can paste an external URL in the Page Link block:
But I'm increasingly not sure the distinction between the two is valuable, since they both do the same. And by adding additional "link type" blocks, it will only serve to make the two-step flow that much more clunky. Have you considered, instead of registering new link type blocks, to extend or expand the URL dialog instead? Instead of this one:
In the end, a menu item is just a text label and a link, with affordances for choosing that link. We might just want to let you pick that link interface? This is a rough sketch that needs time in the oven (perhaps something better than a dropdown to choose the type), but hopefully it illustrates the idea:
See also #24099. |
|
Thank you all for the feedback and ideas!
I don't have any data, but from my tests with WooCommerce, BuddyPress and BigCommerce, it looks like they add less than a dozen links each.
Do you have an example of what "within a script or the block variation data itself" would look like? That's an approach I experimented with in the beginning, but couldn't figure out how to get working. Is your proposal similar to @spacedmonkey's to use a localize script?
Sure thing, that's one of the 'alternative implementations' that I considered in #31316. I didn't take that route for a couple of reasons:
Notice none of these reasons are blocking and I'm completely fine exploring the route of extending the URL dialog instead of creating new Navigation Link block variations, but I wanted to expose some of the reasons that made me try the latter approach first. |
Thank you, that's awesome. I'm trying to go back to the initial use case meant to be solved by this. It may have made sense for a dedicated navigation screen that is primarily intended for organizing navigation items. But the new navigation block isn't just links. In that context, having to pick between multiple different types of links, I'm not convinced, is the best user experience. It makes sense for the "Home Link" as that's kind of a disparate thing with little to no configuration. But even the separation between Page Link and Custom Link is spurious. To be clear, separate blocks might still be the correct path forward for the API. But it's a path I suspect that should be paired with improvements to the quick inserter, and yes, possibly also improvements to the link dialog. I'd appreciate a wider range of thoughts here: @draganescu @mcsf if you have time, thank you. |
@Aljullu this depends on what flow we ultimately end up with. I'd see if we can get folks to align on a user flow in the block editor first. What's important here is that if most plugins only need a few items, both an API endpoint and some other mechanism like passing an array of links in a hook can be viable.
@jasmussen thanks for the insights here! Each custom post type may also be registered as a navigation block variant, so I wonder if we'll need to help fix this in future PRs. Going back to this PR. One thing I'm reminded of is that since we're working with static link content, I'm actually wondering if a block pattern is a better fit. 🤔 Perhaps we could apply a pattern as an option to the initial navigation placeholder? https://developer.wordpress.org/block-editor/reference-guides/block-api/block-patterns/ |
| @@ -105,6 +105,13 @@ Whether to present suggestions when typing the URL. | |||
|
|
|||
| Whether to present initial suggestions immediately. | |||
|
|
|||
| ### noDirectEntry | |||
There was a problem hiding this comment.
Ah, that's unrelated to the other changes in that PR. It's a prop that was undocumented. I thought all props had to be in the docs, but happy to remove it from here if not.
|
TLDR We should probably drop this link block variation idea altogether. The path in this PR is basically using the same system that the default variations use, so I don't think it's wrong. As a user I want to add a link to "place" by adding a link block and linking it to "place". If "place" is not an URL the link block should do its best to suggest the best match, based on me entering "place" in the search box. Based on this story, we should allow users to search for Home, Front page, Orders, Cart, Members, Profile, All forums or whatever kind of special places the system provides, and which have dynamic, generated locations. Just as the user can search in posts or pages by their title. I don't think these variations are a great win. We did copy the way the old screen behaved, but TBH, that screen did not have a live query UI which can be extended to respond quite smartly. We now have a single entry point, a search box, the most common UX. Why do we need to feature "post link", "custom link", "orders link"? Is the win worth the problem? We also used a variation so that we can show default suggestions that match. Say I insert a Page link I see most recent pages. But is this extra step worth it considering I have a search box which looks through all the "stuff" on my site (ideally it should search everything, including media)? Don't we just create an assumption for no reason that all "link types" (a concept which is foreign to the web) should now have a button? The only other problem still remaining is that all links except custom (aka manually entered) links shoud not be static as they're depending on various system settings. |
packages/core-data/src/fetch/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
|
Agree with the general consensus that the number of variations exposed in the inserter can be confusing. I don't think it'd be a problem to hide most or all of them from the inserter, the type of link selected from the link interface can (and I think already does) determine the variation. The linking interface might need some improvements to cater for the amount of items a user can search for. That could probably tie in with the possibility of bulk adding links - #31667. |
Aljullu
force-pushed
the
fix/31316-honor-customize-filters-navigation
branch
from
17d4d17
to
6e73864
Compare
Aljullu
force-pushed
the
fix/31316-honor-customize-filters-navigation
branch
from
6e73864
to
5c34f3b
Compare
|
Thanks for the feedback and all ideas in this PR. I applied some of the suggestions, but my understanding is that some others need some design/product decisions. I tried to gather all ideas below (please, feel free to add new ones or edit them if you feel I didn't explain them right):
I will need some guidance on moving this forward because I don't think I'm entitled to make a decision on what approach is better. Also, I think it would be great to distinguish what should be fixed as part of this PR and what can be done as follow-ups. |
|
Thanks for all the work here. I'd like to offer thoughts on at least what we should avoid:
At the same time, consolidating on a single Menu Item block doesn't really work that well unless paired with an upgraded URL picker. So to an extent there's a bit of a meta conversation, essentially repeating what @draganescu said:
If we think the latter, then it should be blocks using the existing transform API, so we avoid building stuff just for this one block. Even if we find it necessary to go with blocks (whether for API or pragmatic/move forward reasons), improvements to the quick inserter can help. For example we might want to tailor the default block suggestions, and only surface others when searched for. @mtias had thoughts on this with the Home Link block, curious what your take is. |
|
@Aljullu there hasn't been much activity on this PR in a while, just checking if there is still a plan to continue working on this. I have removed the Navigation Screen label since the project was moved to an inactive status on the feature projects page in coordination with the project leads. However, the work can be continued with the navigation block. |
I think we can close this PR given the feedback that was provided, I experimented with an alternative approach in #35876. Thanks for the heads-up, @Thelmachido! |
Description
This PR adds support for the
customize_nav_menu_available_itemsandcustomize_nav_menu_available_item_typesfilters in the navigation block and navigation screen.Fixes #31316.
In order to do so:
customize_nav_menu_available_item_types.customize_nav_menu_available_itemscan be queried by the LinkControl component.Note: I didn't include any tests yet, before I would like to validate if the approach taken in this PR makes sense.
Testing steps
You can either test with the code snippet example I posted or with real plugins (see section below).
With an example
Paste this PHP code somewhere, simulating it's a plugin adding some nav items:
Repeat the process in the Navigation screen and widget screen with a classic theme and in the Site Editor with a block-based theme.
With real-life plugins
WooCommerce
Currently, WooCommerce only adds the
customize_nav_menu_available_itemsfilter in admin screens, that makes those items not to be available in Rest API calls. I created a branch in WooCommerce's repo which would change this, so in order to test, you will need to check out that branch.wp-content/pluginsfolder clone WC (git clone https://github.com/woocommerce/woocommerce)woocommercefolder and checkout the correct branch (git checkout fix/menu-items-in-rest)Repeat the process in the Navigation screen and widget screen with a classic theme and in the Site Editor with a block-based theme.
BigCommerce
I had trouble enabling BigCommerce in a WP 5.8 site, so what I did was to install BigCommerce in a WP 5.7 and then update WP to 5.8.
Once you have BigCommerce enabled in WP 5.8 (you don't need to create an account to test this PR):
Repeat the process in the Navigation screen and widget screen with a classic theme and in the Site Editor with a block-based theme.
Regression testing
It would be good to test other Navigation Link variations to ensure there are no regressions in them:
Screenshots
WooCommerce menu custom items in the Navigation screen:
BigCommerce menu custom items in the post editor:
Types of changes
New feature: register Navigation Link block variations based on the values returned by
customize_nav_menu_available_itemsandcustomize_nav_menu_available_item_typesfilters.Checklist:
*.native.jsfiles for terms that need renaming or removal).