Default suggested links to pages #54622
Conversation
|
Size Change: +569 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
draganescu
added
[Type] Enhancement
|
I found it very surprising the depth of where this change had to be made, I expected this to be changed either in the consumer (e.g. navigation block) or in the component (LinkControl, LinkUI, search something) - but it was all the way down in core data. I think this is from a time when things were simpler and pherhaps today we could bring the "concerns" more together. |
|
Very nice! Seems like it just needs a code review, this'll be good so we don't see recently uploaded fonts suggested here 😅 Thanks Andrei. Almost certainly separate, but I would love for a next step to be showing these pages hierarchically and scrolling, i.e. a tree view so you can see the structure of your site rather than the most recent n pages. This is mentioned in the main tracking issues and is relevant mainly because pages are, arguably, mostly things you publish initially and once, and then they stay static for a long time. To that end, if we only show the most recent pages, you'll in most cases always see the same suggestions. |
It exposes APIs which you can hook into which allow you to define the search handler it uses when looking up suggestions. For WordPress specific use that handler is in Core Data. I'm not sure we can bring the concerns closer together but I'm interested in any suggestions - perhaps in a dedicated Issue? |
There was a problem hiding this comment.
Thanks for this PR. I agree with the goal to prioritise the most appropriate content type. However I have some concerns relating to how this will impact beyond the Core implementation so I'm raising a blocking review to avoid this landing for now...
Let's discuss further. Thanks 🙇
defaults suggestions in LinkUI to pages.
Forgive me but I don't think this is actually what is being achieved by this PR. The modification is to make the initial default search suggestions shown by the control on render prioritise Pages.
See screencapture below which shows the behaviour I describe above:
Screen.Capture.on.2023-09-20.at.11-31-47.mp4
That seems to be different from the stated goal of the PR but please let me know if I'm wrong.
| queries.push( | ||
| apiFetch( { | ||
| path: addQueryArgs( '/wp/v2/search', { | ||
| search, | ||
| page, | ||
| per_page: perPage, | ||
| type: 'post', | ||
| subtype, |
There was a problem hiding this comment.
As state in the PR description this is a change across WP Core in how default links behave. This also applies to any 3rd party consumers of the component which utilise the fetchLinkSuggestions function from @wordpress/core.
Now all initial suggestions are forced to Page. There may be consumers of this component for whom that change is not what they wish and I believe it represents a potentially breaking change.
We should be absolutely certain this is what we want for all consumers before we merge this change.
There was a problem hiding this comment.
The problem with the way current initial suggestions work is that they are not meaningful. It would be an enhancement in terms of UX if the initial suggestions would always list pages. Ideally the initial suggestion query should be open to 3rd parties so they can fit their needs.
As it is now we show "a bit of everything" which doesn't really serve any purspose.
Do we have an Issue tracking this problem? I'd like to replicate in order that I can address this. |
|
@getdave thanks for checking this out. I think this is what we want at least as a first step:
I think what I call default you call initial. Later we should want to improve this even further and achieve something like this - a visual tree of the page hierarchy - as the 1st step when creating a link that has suggestions. It is still unknown what should happen for a website which has no pages or one page, because the website uses custom post types or simply just posts exclusively. What do you think @jasmussen , should we keep the current behavior if there are no/too few pages?
Check this out from Joen's previous issue.
Yes. That is true. It makes me think that the initial (default) suggestions should be controlled at the block level and we should not rely on a public API for this so we can cutomize the UX without having to cater to 3rd party contracts. So I should not change the fetching core data suggestions and instead add fetching in the navigation block, whay do you think @getdave ? |
|
I think initial page-specific suggestions are good, but other link variations should still maintain their suggestions — i.e. categories, tags, posts. This PR:
Trunk:
|
|
Related: We should account for page suggestions on LinkControl generally next: #50884 |
|
Good catch @richtabor. So a better way of formulating the need is:
I'll rewrtite this PR with this goal. |
Please can we try and make whatever we implement geared so that it's not specific to any block or WordPress? Whilst this function is part of I think it could be formulated as: "the initial suggestions should default to the type configured when calling the search handler" 🙏 Thank you |
I think perhaps this is being lost in translation? 😄 The PR says "defaults suggestions in LinkUI to pages". To me this means: "this PR sets the default value for the suggestions in the Link UI to be pages". Based on your comments, perhaps a more accurate wording could be: "This PR changes the default type for the initially shown suggestions to always be "Pages"? Sorry if this sounds pedantic but I wouldn't want people to think this PR solves more than it actually intends to 😅 |
Isn't this already the case, for all other variations (related: #54668). I think it's just the custom link that needs the page suggestions (on this effort). |
draganescu
force-pushed
the
try/default-to-pages
branch
from
1866bb0 to
bedbd1f
Compare
|
So, considering the way suggestions work:
For custom links the empty query makes for searching literally into any type of content. While this is expected in general for links it's questionable if for the navigation block it is expected (to see links to fonts or other types of media/attachments as a "menu" item). To quote @getdave
... which is bad because @richtabor says other "types" should show suggestions that are conforming to their type (a category link should show category suggestions). Therefore the idea would be to have pages suggested for custom links and page links in the navigation block, but custom links to still search in everything. For this the way suggestions work would need to be updated to support an optional different query for suggestions. As if the whole link component tree is not complicated enough as it is. Another option would be to support another search query prop, which if it is undefined means the suggestions query is the same as the search query but without any search prop, and if it is set it is used for suggestions. I think this second option makes more sense given the current implementation. |
|
The fetch handling method has a prop under Why don't we use that to provide a different query? If that route isn't available then I guess we'll need a new prop but we really need to make every effort to avoid expanding the API of |
|
@getdave that would make the type of isInitialSuggestions be
Edit: actually not really, |
I'd got for that additional API then 👍 |
This reverts commit bedbd1f.
…s for initial suggestions on link UI
| @@ -44,7 +44,15 @@ export function getSuggestionsQuery( type, kind ) { | |||
| if ( kind === 'post-type' ) { | |||
| return { type: 'post', subtype: type }; | |||
| } | |||
| return {}; | |||
| return { | |||
| // for custom link which has no type | |||
There was a problem hiding this comment.
| // for custom link which has no type | |
| // for custom link which has no kind |
Is it kind?
There was a problem hiding this comment.
It's type. The switch is on type and the default is for no type.
|
Having spent a while looking at this previously I can see that this is a very elegant solution, well done! |
There was a problem hiding this comment.
Great job on this. Thank you for continuing to wrangle it.
I like this PR because it has avoided bloating the API of LinkControl whilst still allowing us to achieve what we need in an way that is isolated to the Nav block but gives option for extension in the future 👍
I hope you don't mind but I've pushed a few changes:
- added unit test coverage for changes to
fetchLinkSuggestions - fixed potential bug with
undefinedvalues. - renamed the new option to
initialSuggestionsSearchOptionsfor consistency.
I think we should also update the description of the PR to make sure it is current with what is going to be merged here. The main thing is making it clear that this changes only effects the Navigation Link block and not the entire editor (unless I misunderstood something?).
Other than that LGTM 🚀
packages/core-data/src/fetch/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
packages/core-data/src/fetch/__experimental-fetch-link-suggestions.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in 9bd8998. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6356477842
|
|
Updated the PR description. |
|
Haven't we tried this approach before, but it got reverted eventually, or am I miss-remembering something? |
What?
Closes #53904 - defaults suggestions in LinkUI to pages.
~~This change applies in all linking instances where linking UI shows suggestions, not only in the navigation block as the original issue asks.
I think this is good for consistency and also because the mixed suggestions seem less likely to be helpful.~~
Why?
Because most linking is either towards pages or external links.
For taxonomy links or posts it is far more likely to need to filter by search term.Pages are generally few in number and therefore it makes sense to always default to suggesting existing pages.
How?
This is how the suggestions are setup and shown in the navigation link block, part of the navigation block:
~~This exploration changes how the core data (!)
fetchLinkSuggestions... util? works. This currently searches through everything and tries to show something.The change defaults to always showing pages if the call is for "initial suggestions" - which also means:
The PR introduces a new member of the search options object that gets eventually used by
fetchLinkSuggestions. This newinitialSuggestionsSearchOptionsmember overrides all the options insearchOptions;for the initial suggestions.The navigation link block now uses this to default to suggesting a max of 20 pages initially for the custom link variation.
Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
page-suggestions-default.mp4