-
-
Notifications
You must be signed in to change notification settings - Fork 287
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: duplicate entries in search query #3289
fix: duplicate entries in search query #3289
Conversation
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.
As easy as that! Thank you @anshpathania7!
Not 100% related: your commit history is a bit messy - like the 11 commits needed for this PR. Please clean this before your next PR.
Codecov Report
@@ Coverage Diff @@
## develop #3289 +/- ##
===========================================
- Coverage 10.50% 10.50% -0.01%
===========================================
Files 249 249
Lines 12296 12297 +1
===========================================
Hits 1292 1292
- Misses 11004 11005 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Yes yes, I'll definitely clean up commits before further PRs :D |
@@ -19,7 +19,8 @@ void _performSearch( | |||
String query, { | |||
EditProductQueryCallback? editProductQueryCallback, | |||
}) { | |||
if (query.trim().isEmpty) { | |||
query = query.trim(); |
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.
Just curious , we aren't handling the "redbull" and "Redbull" as two different things here
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.
@AshAman999 The fix was only about spaces, not lower / upper case.
"redbull" and "Redbull" will be considered as different.
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.
So we're going to handle that case or leave it as it is for now ?
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.
@AshAman999 I think "redbull" and "Redbull", both should be treated as separate keywords as on UI side they don't look same (also There could be some brands having capital keywords?).
In search history, "redbull" and " redbull " were treated as separate keywords but to user they would look same (because couldn't tell difference between whitespaces), hence duplicate.
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 pops a question in my mind that should there be a some sort of suggestion in search box ? if user inputs something like "REdbUll" or "r ed bul l", then should there be suggestion for correct word?
Maybe this could be a feature?
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.
Yeah some kind of local autocomplete would make sense. Though as far as I can remember we only store 5 or so last searches atm.
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.
If you guys have suggestions beyond this PR feel free to open a new issue, so that we can merge this successful PR first.
Seeing several times the exact same text (e.g. "redbull") (modulo the spaces) was a bit problematic, and that's what the PR was about. Done.
What
Fixes bug(s)