Closed dpasque closed 1 year ago
Hey @karenroldan! 😄 Thanks for the review!
thought: I wonder if we should tokenize the search term and see if it matches one of the keywords. Right now, keyword matching only works when there's one search term. What do you think?
Absolutely!! I love this idea and had a similar thought while I was working on this! 🎉 But I think we should do that in a separate PR, because it's a slightly bigger change as we'll have to update our searching and indexing logic (the token lookup should probably happen in the inverted index). I'll spin up an issue for our Backlog 👍
Hey @dpasque 👋 Sorry for the late review! Everything looked great from my testing.
question (non-blocking):
I noticed that the highlighting chops up keywords/descriptions into different spans
, which means VoiceOver reads them separately (e.g. Payment Blocks is read as "pay, ment blocks"). I feel like this is probably expected (maybe similar to how things are read for the result rows on duplicate search), but I just wanted to double check!
@john-legg gah, sorry! I saw Karen's review and just plowed ahead lol 🤦
I noticed that the highlighting chops up keywords/descriptions into different spans, which means VoiceOver reads them separately (e.g. Payment Blocks is read as "pay, ment blocks"). I feel like this is probably expected (maybe similar to how things are read for the result rows on duplicate search), but I just wanted to double check!
Great question, and you're spot on! It's the same thing as the search results, and (AFAIK based on my research) is just a normal and expected (and non-disruptive) side-effect when you have sub-spans 👍
What Does This PR Add/Change?
The goal of this PR is to ultimately add search highlighting to all the goodies in the selected feature panel! (Namely, keywords and descriptions).
To get there, there were a few other tweaks/fixes to be made:
TextMatchHighlighter
where spaces where getting gobbled up sometimes. This is because of how CSS handles whitespace collapsing! In short, spaces at the start/end ofspans
weren't displaying. To get around this, we are now applying thewhite-space: pre-wrap
style to all those spans.Testing Instructions
REACT_APP_REPORTING_CONFIG_NAME='user-testing.json' yarn start
. Let me know if you need a copy of the user-testing JSON.Issues
Related to #
Closes #97