Feature/snow 352 add top queries component #94
Conversation
|
Can we have a nice description 😄 |
mikegron
requested review from
jeremierobert-coveo,
nathanlb,
tedre191,
mikegron,
lbergeron and
louis-bompart
| const a = document.createElement('a'); | ||
| a.classList.add('coveo-link'); | ||
| a.addEventListener('click', () => { | ||
| this.usageAnalytics.logSearchEvent(TopQueries.topQueriesClickActionCause, {}); |
There was a problem hiding this comment.
I don't see the search performed by this component.
Are you sure that even won't get dismissed ?
| title: ComponentOptions.buildStringOption({ defaultValue: l('TopQueries_title') }), | ||
| onClick: ComponentOptions.buildCustomOption((s) => null, { | ||
| defaultValue: (expression: string) => { | ||
| window.location.href = `#q=${expression}`; |
There was a problem hiding this comment.
Instead of playing directly with the URL maybe we should use the QueryStateManager ? 😸
… generaly improved tests.
pages/top_queries.html
Outdated
| <script class="coveo-script" src="../js/CoveoJsSearch.Lazy.js"></script> | ||
| <script class="coveo-script" src="../commonjs/CoveoJsSearchExtensions.js"></script> | ||
| <script> | ||
| let attachedIds = []; |
There was a problem hiding this comment.
Where is this used?
|
|
||
| let suggestions: IQuerySuggestResponse; | ||
| try { | ||
| suggestions = await this.queryController.getEndpoint().getQuerySuggest(suggestionQueryParams || this.options.suggestionQueryParams); |
There was a problem hiding this comment.
That finicky, but whenever possible, I think you should prefer nullish coalescing over the logical OR operator.
That's flavour mostly, but I think it's more 'explicit'.
| let topQueries = Mock.basicComponentSetup<TopQueries>(TopQueries, options); | ||
| let stub = sandbox.stub(topQueries.env.searchEndpoint, 'getQuerySuggest'); |
There was a problem hiding this comment.
You copy-🍝 the setup phase of your tests a lot, you may want to move them to a beforeEach close or a function.
The balance between Do Repeat Yourself in test and the classic Don't Repeat Yourself is finicky, so that could be considering an opinion (i.e. feel free to ignore me on this one ;P )
There was a problem hiding this comment.
Thanks a lot for all the great feedback!😄
Co-authored-by: Jeremie Robert <43446516+jeremierobert-coveo@users.noreply.github.com>
Co-authored-by: Louis Bompart <louis.vincent.bompart@gmail.com>
Co-authored-by: Louis Bompart <louis.vincent.bompart@gmail.com>
Co-authored-by: Louis Bompart <louis.vincent.bompart@gmail.com>
Co-authored-by: Louis Bompart <louis.vincent.bompart@gmail.com>
… in the suggestion query
| static options: ITopQueriesOptions = { | ||
| suggestionQueryParams: ComponentOptions.buildJsonOption<IQuerySuggestRequest>({ defaultValue: { q: '' } }), | ||
| title: ComponentOptions.buildStringOption({ defaultValue: l('TopQueries_title') }), | ||
| onClick: ComponentOptions.buildCustomOption((s) => null, { |
There was a problem hiding this comment.
Why do you have a lambda that strictly do nothing ?
There was a problem hiding this comment.
This argument translates a string option to a specified type. Since that type is a function, we only accept it beeing defined in JS when initializing the component.
Co-authored-by: Jeremie Robert <43446516+jeremierobert-coveo@users.noreply.github.com>
…ithub.com/wosullivan/search-ui-extensions into feature/SNOW-352_Add_TopQueries_Component
The TopQueries component suggests the most searched queries in the specific context (using a query suggestion with empty q) and links the search results of the suggestions to the user.
The title as well as the action taken on suggestion click can be modified with the options.
There is no UI mock-ups.