fix(frontend/controller): highlight full words in search #536
Conversation
There was a problem hiding this comment.
Thanks for your first PR! A huge fan of the way you've put in and documented your work 👌. Really appreciate the thorough testing and listing out of the cases, along with screenshots 💯
The approach is good (scan left + right to widen the highlight boundaries to whitespaces).
Left some comments, minor changes. I think we can evolve this and make it a little more readable + concise.
We should also note this somewhere in the contributing docs - don't commit package-lock.json as part of a PR unless you've specifically updated or installed a package, since resolving merge conflicts across merged branches in the file is a pain (especially when npm i will just regenerate/update it for you)
|
|
||
| /** | ||
| * Separates the line into words before the first match, the first match, and after the match. | ||
| * TODO: Extend this logic to find every match, rather than simply the first one. |
There was a problem hiding this comment.
What did you think of this suggestion? Should I create an issue or is it not the correct functionality/so low pri that it's not worth documenting? Normally my workflow would be to create an issue and reference it in this TODO
There was a problem hiding this comment.
Ah, I wasn't quite sure why/where else we'd apply this, but I now see that the case that it might apply is when you've got two of the same word within the string. However, I still do not believe we'd ever want to highlight more than what's actually been entered, since a single entry of ihr (har) will not yield a higher match to ihr ihr ihr gun gwvou compared against ihr gun gwo mnw etc.
@bhajneet perhaps you can validate my assumption.
There was a problem hiding this comment.
I'll take this TODO out for now.
|
@Harjot1Singh PTAL |
| // will be -1. In this case, we want to end at the last position in the line. | ||
| const matchEndPosition = wordEndPosition === -1 ? foundPosition + query.length : wordEndPosition | ||
| return [ | ||
| line.substring( 0, matchStartPosition ), |
There was a problem hiding this comment.
Why did you use substring here over slice out of interest? It'll definitely function the same way, just curious. In fact, by doing so, you no longer need L66, since the behaviour of substring is to treat any negative or NaN arguments as 0. So, technically you could scrap L66 entirely, and move the ternary for matchEndPosition to where wordEndPosition is declared (thus, two less variables declared, less cognitive overload).
There was a problem hiding this comment.
Why did you use substring here over slice out of interest?
I mostly did it out of cheekiness in the sense that there's no reason to prefer one over the other and I like substring more 😂 but then I didn't want to modify what existed too much so slice was used below. The removal L66 is a good enough reason for me, I'll refactor. If you all prefer I use slice happy to do that too.
move the ternary for matchEndPosition to where wordEndPosition is declared (thus, two
less variables declared, less cognitive overload).
This would require me to write out and evaluate the code twice, right? The two options are:
const wordEndPosition = line.indexOf( ' ', foundPosition + query.length )
const matchEndPosition = wordEndPosition === -1 ? foundPosition + query.length : wordEndPosition
or
const matchEndPosition = line.indexOf( ' ', foundPosition + query.length ) === -1
? foundPosition + query.length
: line.indexOf( ' ', foundPosition + query.length )
The second one seems more clunky to me, evaluates the same line twice, and I think the extra variable makes things clearer. Is there another way you are thinking that I'm missing?
There was a problem hiding this comment.
Haha, a nice unintended pro to using substring in that instance ;)
Totally happy with your reasoning and with what you've done above, those are the only two options I can see as well.
|
@Harjot1Singh ready for another look! |
| // will be -1. In this case, we want to end at the last position in the line. | ||
| const matchEndPosition = wordEndPosition === -1 ? foundPosition + query.length : wordEndPosition | ||
| return [ | ||
| line.substring( 0, matchStartPosition ), |
There was a problem hiding this comment.
Haha, a nice unintended pro to using substring in that instance ;)
Totally happy with your reasoning and with what you've done above, those are the only two options I can see as well.
Summary
Closes #170. Does not change the functionality of the
firstLettersearch highlighting, but modifies thefullWordhighlighting to include all words.Before
After
First letter search still works
How was this tested?
I tried the following test cases:
Happy to write unit tests once those exist (if they exist).
Other odds and ends
I didn't have permission to push to ShabadOS/desktop, and the instructions said to fork so that's what I did - not as well versed with the workflow so let me know if I need to change things.
I was surprised at some of the eslint rules, especially the extraneous semicolon being enforced.
Reviewers
@saihaj @Harjot1Singh