[NO-CI] Update search results loaded event #356
Conversation
jjl014
left a comment
jjl014
left a comment
There was a problem hiding this comment.
Lots of changes in this PR, but things are mostly looking good from what I can tell. I did leave some comments around a few failing tests and some things that we may want to keep around. Lmk what you think. Thanks!
It might also be a good idea to get another review of the changes just in case I missed anything. 👍
| }); | ||
|
|
||
| it('Should respond with a valid response when term and required parameters are provided', (done) => { | ||
| it('V2 Should respond with a valid response when term and required parameters are provided', (done) => { |
There was a problem hiding this comment.
This test is failing for me due to the server returning a 400. Getting items[0].item_id must be a string
| numResultsViewed = num_results_viewed, | ||
| items, | ||
| analyticsTags, | ||
| resultCount = result_count || items?.length || 0, |
There was a problem hiding this comment.
Do we want to keep some of these changes, like this one, that we added in previously?
src/modules/tracker.js
Outdated
| if (items && Array.isArray(items) && items.length !== 0) { | ||
| const trimmedItems = items.slice(0, 100); | ||
|
|
||
| transformedItems = items; |
There was a problem hiding this comment.
Should we keep the trimmage?
src/modules/tracker.js
Outdated
| selected_filters, | ||
| selectedFilters = selected_filters, | ||
| url = helpers.getWindowLocation()?.href || 'N/A', | ||
| url, |
There was a problem hiding this comment.
Should we keep this change as well?
src/modules/tracker.js
Outdated
| num_results, | ||
| result_count, | ||
| resultCount = result_count, | ||
| numResults = num_results || resultCount, |
There was a problem hiding this comment.
Hm... this doesn't seem to work very well when you end up with:
num_results = 0 and resultCount = undefined
This actually comes out to be undefined and is causing one of the tests to fail: Should respond with a valid response when term, and zero value num_results parameter are provided
I think we can probably use nullish coalescing here and do num_results ?? resultCount. If num_results has a value (even if it's 0) and it's not null, it'll use that value, otherwise it will use resultCount
src/modules/tracker.js
Outdated
| customer_ids = customerIds, | ||
| itemIds, | ||
| item_ids = itemIds, | ||
| resultCount = numResults || result_count, |
There was a problem hiding this comment.
We should probably use nullish coalescing here as well: num_results ?? result_count
| url, | ||
| section, | ||
| analyticsTags, | ||
| resultCount = numResults || result_count || items?.length || 0, |
There was a problem hiding this comment.
Should we keep this change as well?
jjl014
left a comment
jjl014
left a comment
There was a problem hiding this comment.
LGTM!
Just left one minor comment/suggestion, but I think this is ready to go. Thank you for working on this!
Co-authored-by: Jimmy Li <jimmyli921@gmail.com>
No description provided.