-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix/content picker improvements #19
Conversation
…result if we are only searching one type
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.
@tlovett1 The changes are working well, but I couldn't resist a few higher-level suggestions. Feel free to ignore those as they can be tackled separately. The only one I would call required is moving the search cache into state for the reason described in my inline comment.
components/ContentSearch/index.js
Outdated
@@ -51,30 +53,37 @@ const ContentSearch = ({ onSelectItem, placeholder, label, contentTypes, mode, e | |||
setSearchString(keyword); |
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.
This may look like overkill and is totally optional for this PR, but once the search cache is converted to state, we can handle search changes a little more efficiently (and close #18) by responding to keyword changes with effect hooks.
/**
* handleSearchStringChange
*
* Using the keyword and the list of tags that are linked to the parent block
* search for posts/terms that match and return them to the autocomplete component.
*
* @param {string} keyword search query string
*/
const handleSearchStringChange = (keyword) => {
setSearchString(keyword);
}
useEffect(() => {
if (isLoading || searchString in searchCache) {
return;
}
setIsLoading(true);
const searchQuery = `wp/v2/search/?search=${searchString}&subtype=${contentTypes.join(
',',
)}&type=${mode}`;
apiFetch({
path: searchQuery,
}).then((results) => {
const newResults = results.filter((result) => {
let keep = true;
if (excludeItems.length) {
excludeItems.forEach((item) => {
if (item.id === result.id) {
keep = false;
}
});
}
return keep;
});
setSearchCache(oldSearchCache => ({...oldSearchCache, [searchString]: newResults}));
setIsLoading(false);
});
}, [isLoading, searchCache, searchString]);
useEffect(() => {
if (searchString in searchCache) {
setSearchResults(searchCache[searchString])
}
}, [searchString, searchCache])
The first effect hook will run when the search term changes, when the cache changes, and when isLoading
changes. This includes when isLoading
becomes false -- guaranteeing that the latest search term will always be queried. But by having that isLoading
check at the start of the effect hook, we also ensure that only one fetch request is running at a time.
The second effect hook responds to changes to the search cache. This will run after a successful fetch
to populate the results.
Great feedback @johnwatkins0 . I pushed up some changes. I left the search cache outside of the component because I think it's really important to share search cache across multiple instances. |
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.
Looks good.
Description of the Change
Possible Drawbacks
Addresses #18