Navigation panel: Add search to content menus #27280
Conversation
david-szabo97
requested review from
vindl,
noahtallen,
mattwiebe,
Addison-Stavlo,
Copons and
jeyip
| { showOnFront === 'posts' && ( | ||
| <NavigationItem | ||
| item={ 'post-/' } | ||
| title={ __( 'All Posts' ) } | ||
| onClick={ onActivateFrontItem } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
All Posts is not included in the search results. Should we also include this?
There was a problem hiding this comment.
I think it's fine to omit it. Furthermore I'm not sure if we should be displaying it in posts menu anyway since it sounds more like page template to me. This should probably be reserved just for individual posts and not a collection of them.
|
Size Change: +479 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
david-szabo97
force-pushed
the
add/navigation-panel-content-search
branch
from
ba283b4 to
abc0c13
Compare
|
We have the same problem as we did with template and template parts menus search. We dropped the debounced search for those menus, but we definitely need them for content menus. See #27160 for more information. This PR is blocked till a solution is merged, opened a PR which is a quick but a bit smelly fix: #28102 |
david-szabo97
force-pushed
the
add/navigation-panel-content-search
branch
2 times, most recently
from
f09f3d5 to
df3a37a
Compare
Complementary PR for #27280
david-szabo97
force-pushed
the
add/navigation-panel-content-search
branch
from
df3a37a to
d47d320
Compare
Now that the noted PR is merged, is this unblocked and ready for review now? |
| 'postType', | ||
| 'post', | ||
| { | ||
| search: searchQuery, |
There was a problem hiding this comment.
I think that this will implicitly search the content of the posts too. My expectation was that the search will only be applied to post titles so as to make it consistent with existing template searching, but this might make sense too. What do you all think? 🤔
There was a problem hiding this comment.
My expectation was that the search will only be applied to post titles so as to make it consistent with existing template searching, but this might make sense too.
Same thought here. While I would expect it the component to search only by titles, returning results for some matching content may be nice! Although if we go with that we should probably ensure the results are prioritized with those matching titles to show up first.
There was a problem hiding this comment.
As far as I know, there is no way to search only in the title. Is there? 🤔 We might need to come up with a custom endpoint if we want to search by title only or do the prioritization.
There was a problem hiding this comment.
I don't think its a bad thing that we search content, its a feature not a bug! However, I do have a complaint about how it orders things. The results seem to be ordered based on the date of the post. So if I am look for 'example', the posts with the word 'example' in their title show up after more recent posts that have the word 'example' in their content:
I guess my recommendation would be to sort the results such that results with titles corresponding to the search are at the top.
There was a problem hiding this comment.
Pushed a commit which puts results with titles at the top.
| 'postType', | ||
| 'page', | ||
| { | ||
| search: searchQuery, |
There was a problem hiding this comment.
Same as my remark for posts below: https://github.com/WordPress/gutenberg/pull/27280/files#r563820929
Sorry, missed your message! Yes, it's ready. |
...ges/edit-site/src/components/navigation-sidebar/navigation-panel/menus/content-categories.js
Show resolved
Hide resolved
david-szabo97
force-pushed
the
add/navigation-panel-content-search
branch
from
2edd27d to
6849679
Compare
|
Rebased to pass tests |
There was a problem hiding this comment.
The content search is looking good to me!
The one minor complaint I have (non-blocking and could be a follow up) is that the "Loading..." disappears very quickly and there is a bit of a delay where nothing is shown before results/ no results:
However, it seems like there is a new issue with the templates search being introduced. On master when I use the templates search it only shows the results matching the search, while on this branch when I use the templates search it shows the matching results at the top and ALL other templates underneath it:
Im not sure if this was intentional or some side effect of the changes here, but it seems like the behavior should be consistent between template and content searching? 🤔
Im also noticing there is no need for the "Loading..." in the templates search since results are instant (I assume we are filtering the list that is already there?). This makes me wonder if we should just be filtering the page/post titles that are already there instead of making a request and also searching content (don't have a strong opinion here as the content does add some benefits 🤷♀️ ).
That was actually bug. Fixed it. You will notice "Loading..." still disappears quickly if you are hitting a cached search. (Like searching for "a", then "ab" then "abc". Then start deleting the letters and you will notice the Loading only appears for the debounced time (75ms) since the requests are cached.
It was a side effect 😬 Fixed it |
In the templates search you should only see "Loading..." if the request is still in progress. We are filtering on the client side for templates and template parts.
Loading all posts on the client side would be way too much. Imagine a news site with thousands of posts. This is the reason for filtering on the server side. |
|
@david-szabo97 can we merge this PR after resolving the conflicts? |
Resolves: #26513
Description
Add search functionality to Pages, Posts, and Categories menus in the Navigation panel.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: