New search functionality #265
Conversation
There was a problem hiding this comment.
On first pass
- linter is saying varietyRoles on line 15 isn't used, is it necessary? If not lets remove it.
- Lets also get rid of the console logs.
- I tried testing "Thank" !"you", looking for sentences that has only "Thank" but not "you" and it errored out, unable to read undefined from text. It looks like it's on line 188, I think if there are results from a double quote search but then they all subsequently get filtered out from using !"", it's trying to read from an answer with no results thus erroring out.
- Filtering seems to only be working for full strings right now, is that intended? Also looks like the highlighting is off. It can be adjusted in the TranscriptItem.tsx file. You can look at line 177 to see the Highlighter component.
@evamaxfield
Should we include text that tells the user that they can use quotes and filters?
Should this be case sensitive?
There was a problem hiding this comment.
This may be an issue with my browser/extensions combination, but the feature does not work for me. I am running Chrome, with PrivacyBadger and uBlock Origin. Javascript is enabled.
Also, this is a prime candidate for tests. The logic in it is complex enough that I can't tell if it works at a glance, but you could
- split this chunk of code into smaller, testable chunks
- set up some mocks with test queries (with quotes, limiters, etc)
- test each small chunk to make sure it does what you think it does
- test the code as a whole to make sure it works
There are some testing files in the repo already, use them to see best practices.
| newTerm.substring(0, indices[i * 2]) + | ||
| newTerm.substring(indices[i * 2 + 1] + 1, newTerm.length); | ||
| } | ||
|
|
There was a problem hiding this comment.
can you pull everything from line 104-145ish into a utility function that just lives a neighboring folder? e.g. util/, util/, etc? Cleaner and more testable to keep the logic outside of views, as each function can be assessed on its sole purpose.
There was a problem hiding this comment.
Will do this once I fix John's bugs, thanks for the suggestion!
| const tokenizedQuery = removeStopwords(cleanedQuery.split(" ")); | ||
| console.log("3"); | ||
| if (!cleanedQuery || tokenizedQuery.length === 0) { | ||
| // empty query or no valid tokens to search |
There was a problem hiding this comment.
it seems this comment needs to be updated?
Yes absolutely. Please include some sort of helper text. Can be on hover. Can also save that for a future PR though. |
…ltering.
Link to Relevant Issue
This pull request resolves #138
Description of Changes
Adds new search functionality to event transcript search, including strict search (use double quotes), negative search (put a ! before the double quotes), start filtering (put a ^ before the double quotes) and end filtering (put a $ before the double quotes).
For example,
^"1" $"2" "3" !"4"would only show strings that start with 1, end with 2, include 3, and don't include 4.Linked to Forked Storybook Site
Here