-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 filter around fetch link suggestions #35876
Conversation
@getdave @jasmussen wondering if you would have time to take a look at this or maybe find somebody else who can? I'm happy to discuss alternative suggestions as well if you have some in mind. |
Size Change: +48 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
ebc6e86
to
64bc7ae
Compare
Hey, thanks for the PR! Dave would be a good one to chime in on this one as well. In this comment, we discuss the shortcomings of surfacing the most recent 3 published items and then falling back to search, and it contains some mockups that could surface content in a more compact way from various sources. I understand that's a relatively big endeavor, so not a 5.9 thing. But would such a mechanism be an alternative to, or addition, to what's being proposed here? |
Thanks for sharing those mock-ups @jasmussen. I expect they will work well for our needs. How do you envision extensions will hook into the list of available suggested links there? Registering a new Navigation Link block? |
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.
Thanks for the PR @Aljullu.
Overall this seems like a reasonable addition to me. I have a question regarding providing results async.
@@ -154,7 +155,12 @@ const fetchLinkSuggestions = async ( | |||
} | |||
|
|||
return Promise.all( queries ).then( ( results ) => { | |||
return results | |||
const filteredResults = applyFilters( |
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.
What happens if - for example - I wanted to asynchronously retrieve additional results here from an API endpoint? If you wrap the results of the hook in Promise.resolve()
then it will handle both promises and non-promises.
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.
I should reiterate that particular design appears unlikely to land with 5.9. That also means the precise way it works is not fully hashed out, and I'd always defer to a developer as I'm not fully qualified to speak to the innards. But yes, it does seem at the face of it that the particular way to filter based on post type could come from the navigation item type. This also speaks to the need for better transform tools, as per #34041 you'll always just insert Page Links by default, at least in the Navigation block. |
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 also augment the test suite to include tests for this new filter.
Also wondering whether the filter name itself should be in an "experimental" namespace. Are there docs or conventions for that... 🤔
Lastly, I'd like to get a second opinion here from the Core team. I know there has previously been push back on adding Hooks. I should note that we appear to already have a similar filter in the AutoComplete
component.
🤔 Have we considered allowing a full replacement of the component instead, perhaps like MediaPlaceholder? Folks can then use whatever custom endpoints they'd prefer to control the flow after clicking on the inserter. I'd be very hesitant to add a hook to something that is still experimental and one that we're still exploring UX options on. |
2ba98da
to
59ad6fc
Compare
Thanks for following up and pinging other reviewers @getdave! For now, I added tests in 06f306a and prefixed the filter with
Not sure if that question is for me. 🙂 But if I'm understanding it right, you mean plugins would create their own |
I like the sound of that. This might also allow us to try out a small page list tree view component just for the Page Link menu item type, without doing the full refactor? |
This reflects my concern. I would say however, that it's asking a lot for each extender to re-implement LinkControl. I think the bottom line is that LinkControl needs work to become more flexible. This PR would be a sticking plaster over the underlying problem. FYI I'm looking into a few option for flexibility now but in the short term perhaps Kerry's suggestion is best. |
I'm going to close this PR as it's almost two years old and I'm not actively working on it. We still have an issue in the WC Blocks repo regarding this woocommerce/woocommerce#42670, so we might explore alternative solutions in the future if this gets prioritized. Thanks everybody who participated in the discussions! |
Description
This PR adds a filter around the suggestions returned by
__experimental-fetch-link-suggestions.js
. This will allow plugins to add additional custom suggestions to the URL input.Use case: several plugins have custom endpoints and they want to make it easy for users to add links to those endpoints. For example, in WooCommerce we would like to make it easy to link to some endpoint like Orders, Payment methods, Log out...
That's specially important for the Navigation editor/block. In the past, there were hooks for that (#31316), but those hooks are not currently honored by the Navigation block and the PR which added support for them didn't have any movement in the last four months (#31584). So I'm trying a different approach here. 🙂
How has this been tested?
Testing steps
Sync example
Async example
wiki
.Wikipedia
andWikipedia (English)
suggestions appear.Screenshots
Peek.2021-10-22.12-32.mp4
Types of changes
Introduces a new JS filter:
editor.fetchLink.suggestions
.Checklist:
*.native.js
files for terms that need renaming or removal).