-
Notifications
You must be signed in to change notification settings - Fork 133
Add search path and collection type to theme-predictive-search #140
Conversation
this is related to #139 |
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 Thomas. Works as expected - I tested with different languages 👍
I think there's a bit of code duplication, and I'm wondering if it would be better to only pass in the search route as a param, instead of the full predictive search url. Let me know if you want to chat about it.
this.searchUrl = "/search/suggest.json"; | ||
|
||
if(configParameters.search_url) { | ||
this.searchUrl = configParameters.search_url; | ||
delete configParameters['search_url']; | ||
} |
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 is sort of a duplication of what's happening in request.js
on line 14:
var route = searchUrl ? searchUrl : '/search/suggest.json';
I'm wondering if we should be passing in the full url - including /suggest.js
or just the routes.search_url
provided by shopify to the theme. As a theme dev, it's very easy to access that, and we could add suggest.js
here or in request. What do you think?
Maybe something like:
Request.js
var route = searchUrl + '/suggest.json';
theme-predictive-search.js
this.searchUrl = configParameters.search_url || '/search';
if (configParameters.search_url) delete configParameters['search_url'];
...
}
PredictiveSearch.SEARCH_URL = "/search";
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.
Great suggestion! What I did is renaming search_url
to search_path
as we're now partially adding a "path" to the predictive search route instead of the full url /search/suggest.json
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 to me Thomas.
I 🎩 by passing in the French and English routes, as well as passing in no search path at all (defaults to /search
). And tested with the new COLLECTION
type.
Purpose
The goal of this PR is to add an optional parameter when querying the predictive search suggestion. Currently, it is always default to
/search/suggest.json
. This would open allow international stores to use in their native languages. Example:/ja/search/suggest.json
I'm also adding "collection" type to be supported.
Approach
search_path
to the originalconfig
parameter when we instantiate new PredictiveSearchComponent.searchPath
to therequest
function. At first, I wanted to put it at the end of the list of parameters, but it looked strange after theonError
function callback. The best solution would be converting into ES6 and use default valuesIf nothing is put in the
search_path
options, it would default to/search
PredictiveSearch.TYPES.COLLECTION: "collection"
to support collection typeTesting
yarn link
from this branch for boththeme-predictive-search-component
andtheme-predictive-search
folders.yarn link "@shopify/theme-predictive-search-component"
andyarn link "@shopify/theme-predictive-search"
search_path
parameter. Example: