Inserter: Respect block category when searching for blocks #9999
Conversation
jorgefilipecosta
left a comment
jorgefilipecosta
left a comment
There was a problem hiding this comment.
The code changes look good to me and the PR tested well 👍This seems a nice enhancement.
It looks like the end 2 end tests are failing. I think we may be breaking some tests that relied on the previous way inserter filtered items before because locally the tests are also faling with the following error:
adding blocks › Should insert content using the placeholder and the regular inserter
No node found for selector: button[aria-label="Preformatted"]
@jasmussen, @karmatosed any thoughts on this UX change?
|
GIF:
This seems cool with me. It doesn't work in the slash command — though that doesn't have categories so that's fine. But should it? Right now every block has keywords. For example if you search for "image", you get both the image, cover image and gallery blocks. Should we add the block category as an additional keyword? |
tyxla
force-pushed
the
fix/respect-block-category-inserter-search
branch
from
8634a74 to
ab3952e
Compare
Great catch, thanks! They indeed were broken, and it seems they caught a bug - we didn't check safely for the existence of the category before using it. Fixed in 40495a5.
I wasn't planning on having it there. And to be honest, I wouldn't expect the category to trigger a match there - specifically because categories aren't shown in the autocompleters block. But if you think it'll make sense, I'm happy to address that, but probably in another PR. |
|
I'm still very interested in landing this. @jorgefilipecosta @jasmussen I'd appreciate if you can provide some feedback and let me know if there's anything blocking this one. |
|
I think it makes sense for this to land as-is, without worrying about the slash inserter just yet. |
mtias
added
[Type] Enhancement
|
Fine to leave the slash inserter out. |
| export const searchItems = ( items, searchTerm ) => { | ||
| const normalizedSearchTerm = normalizeTerm( searchTerm ); | ||
| const matchSearch = ( string ) => normalizeTerm( string ).indexOf( normalizedSearchTerm ) !== -1; | ||
| const categories = getCategories(); |
There was a problem hiding this comment.
Could we retrieve the categories in the withSelect using select( 'core/blocks' ).getCategories() and then pass them to this function?
I think data retrieval functions of wp.blocks only exist because of historical reasons and they may be removed some day.
Maybe we can also add a test case to the searchItems tests matching using the categories.
There was a problem hiding this comment.
Could we retrieve the categories in the withSelect using select( 'core/blocks' ).getCategories() and then pass them to this function?
Yes, we could, but I intentionally didn't do it - there are several more usages of getCategories() in this file (and not only in this file), so I think rewriting to use withSelect would be material for another PR.
I think data retrieval functions of wp.blocks only exist because of historical reasons and they may be removed some day.
I realize this might be the case, but they don't even seem to be deprecated yet, so in that case I'd prefer staying consistent and following the example of code that's already merged and has proved to work well.
Maybe we can also add a test case to the searchItems tests matching using the categories.
Good call! I've added a unit test case in 11d1b69
|
I've suggested a PR for introducing category support for slash inserter in #12287. |
6 participants
Description
Update the inserter search to respect block categories when filtering search results.
To achieve this, in addition to what we currently do when filtering search results (comparing against the keywords and the block title), we compare against the title of the block's category.
Fixes #9998.
How has this been tested?
This has been tested manually with the following steps:
I can work on adding a unit test if that makes sense.
Screenshots
Searching for "formatting"
Searching for "layout"
Types of changes
Checklist: