-
Notifications
You must be signed in to change notification settings - Fork 687
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
bug: Unwanted content displayed in search suggestion box when there are no results for search #1252
bug: Unwanted content displayed in search suggestion box when there are no results for search #1252
Conversation
This reverts commit 8ae1943.
This pull request is automatically deployed with Now. Latest deployment for this branch: https://venia-git-fork-khoa-le-bugfix-1223-hide-unwante-c705fb.magento-research1.now.sh |
I see this pull request have error when run tests but I don't see any output that help me fix it. Can anyone help? |
|
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 solves the problem perfectly, thank you!
Looking at that conditional though, I'm left wondering if autocomplete.js
shouldn't house this logic instead. In other words, only render <Suggestions>
if we have results (data.products.items.length > 0
).
Here's where autocomplete.js
renders this Suggestions
component:
<Suggestions
products={data ? data.products : {}}
searchValue={value}
setVisible={setVisible}
visible={visible}
/>
And sending in an empty object as the products
prop just doesn't make much sense to me. We need products
to suggest both categories and other products. If we don't have any, we shouldn't render <Suggestions>
at all (because we don't have any!).
That said, this code works fine and I'd still accept it if you don't want to make the change to autocomplete.js
instead.
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.
I see this pull request have error when run tests but I don't see any output that help me fix it. Can anyone help?
Yeah, that output isn't very helpful sometimes, sorry 😬 . You can get more details locally by running yarn test
.
The unit tests for this component do need to be updated to reflect this change:
Update the "renders correctly" and "renders a category list" unit tests
The products
prop these tests are passing in contains an empty items
array which - after your change - will satisfy the conditional and render null
. Instead of updating the snapshot / assertion, please pass in some items
.
Add a test for empty items
array
Please add a test that ensures this component returns null
when products.items
is an empty array.
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.
Makes sense to me. Thanks for your contribution! 👍
Wow, Github's cache really didn't show me any conversation on this one.
No, I wouldn't make that change. I think the logic is fine where it is. |
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.
We need to make sure the tests are updated/fixed before merging. The code looks fine, otherwise but please fix the test! If you need help with this you can always reach out to us on the community slack or you can comment here and we can try to help.
Assigned this to @tjwiebell to handle writing/fixing tests. |
QA Pass. |
Thank you guys helping me out. |
Description
Disable render suggestion component when search query have not results.
Related Issue
Closes #1223.
Verification Steps
How Have YOU Tested this?
Verification steps above!
Screenshots / Screen Captures (if appropriate):