feat: highlight search results #625
No reviewers
Labels
No Label
1000k
100k
10k
200k
20k
500k
50k
5k
75k
backend
blocked:design
bug
dependencies
documentation
duplicate
enhancement
good first issue
help wanted
invalid
P1
P2
P3
question
scope:intl
scope:nip
scope:query_tracing
scope:ux
wontfix
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: Kieran/snort#625
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "fernandoporazzi/snort:highlight-searched-word"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Solves #559
Check screenshot for UI result.
7f859dc70f
to07bb3cca2f
Hey, thanks for working on this issue! Screenshot looks good. I wonder if you tested it with multiple words "snort social" for example as the search results appear to do an
&&
search, so highlighting both "snort" and "social" would make sense.Either way, for all those direct matches this would already be a great improvement.
Hey @Giszmo, it will work for multiple words. However the highlight is just being applied exactly on what was typed.
On the second image, I search for two words in isolation, however they are not highlighted because there is content between them.
I guess I found a solution that will highlight words in isolation. Soon I'll push the changes
07bb3cca2f
to4a5e32d4d4
Done, now this is the result:
Elegant solution in the RegExp.
I thought only
${searchedValue.replace(' ', '|')}
would be enough but if the highlighting changes to maybe yellow background, your solution is needed.LGTM
@ -17,9 +17,10 @@ export interface TextProps {
disableMedia?: boolean;
disableMediaSpotlight?: boolean;
depth?: number;
searchedValue?: string;
props name should be
highlighText
accepting theRegExp
@ -68,3 +69,3 @@
return <ProxyImg src={a.content} size={15} className="custom-emoji" />;
default:
return <div className="text-frag">{a.content}</div>;
return <div className="text-frag" dangerouslySetInnerHTML={{
This is an obvious XSS leak,
dangerouslySetInnerHTML
should NEVER be usedWhat would you suggest as an alternative?
A lib like DOMPurify could get the job done here.
No, look at
transformText
and use a similar approach, maybe adding "style" or another attribute to the text frag entry, ofc you need to create multiple text fragments in that case with different stylesI implemented the changes using the
transformText
approach. Now there is a new type for theParsedFragment
interface.4a5e32d4d4
toa6d5aa9453
a6d5aa9453
tob318a102b6
b318a102b6
to501f8694c5
New implementation submitted. Preview:
@ -25,3 +27,3 @@
const elements = useMemo(() => {
return transformText(content, tags);
return transformText(content, tags, highlighText);
Its better if you dont implement this in the
system
lib, i dont think its of any use to anybody else who might use the lib, you can just copy the function inside here in thisuseMemo
I don't know if I fully agree about moving the logic of this specific fragment inside this
useMemo
. All fragment-related logic now lives in a single place and it's easy to reason. Eventually the project might have unit tests and it's easier to test pure functions outside hooks.If the problem is that there is now a 3rd argument, we could convert
content
,tags
andhighlightText
into a single object and make thehighlightText
an optional property.I just dont see
highlightText
as useful for other people if they wanted to use the lib, that is why i think it should be outsideI tried to co-locate the function
extractHighlightedText
inside thisuseMemo
, however all the content already has a type(hashtag, text, media and so on...) and it's quite tricky then.So if we are to remove the
highlightText
from thetransformText
, we'd have to have the implementation on top of the typetext
fragment directly on the render. I don't know if it's clear what I mean 😅Something like:
That way we remove the highlightText param from the function and fully delegate this to the Text component.
Opinions?
Yea thats probably fine
Implemented and PR is updated!
501f8694c5
to4bdf94e444
4bdf94e444
tob2d17eab25
b2d17eab25
toc223c89045
You need to fix merge conflicts
Sorry for the long time to fix the conflicts. It should be good to go now. Please, checkout the branch and do some more tests.