feat: highlight search results #625

fernandoporazzi · 2023-08-30T10:49:39Z

fernandoporazzi commented

2023-08-30 10:49:39 +00:00

Solves #559

Check screenshot for UI result.

Solves #559 Check screenshot for UI result.

Screenshot 2023-08-30 at 12.41.44.png

369 KiB

fernandoporazzi force-pushed highlight-searched-word from 7f859dc70f to 07bb3cca2f

2023-08-30 10:51:16 +00:00

Compare

fernandoporazzi requested review from Kieran 2023-08-30 10:58:12 +00:00

Giszmo commented

2023-08-30 12:27:53 +00:00

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, 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.

fernandoporazzi commented

2023-08-30 12:51:25 +00:00

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.

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.

Screenshot 2023-08-30 at 14.55.39.png

1.0 MiB

Screenshot 2023-08-30 at 14.50.36.png

1.1 MiB

👍 1

fernandoporazzi commented

2023-08-30 13:55:17 +00:00

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

> 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

fernandoporazzi force-pushed highlight-searched-word from 07bb3cca2f to 4a5e32d4d4

2023-08-30 14:06:50 +00:00

Compare

fernandoporazzi commented

2023-08-30 14:07:35 +00:00

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

Done, now this is the result:

> > 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 Done, now this is the result:

Screenshot 2023-08-30 at 16.05.38.png

1.1 MiB

Screenshot 2023-08-30 at 16.05.45.png

1.0 MiB

Giszmo commented

2023-08-30 15:19:40 +00:00

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.

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.

Giszmo commented

2023-08-30 15:20:23 +00:00

LGTM

Kieran requested changes 2023-08-30 15:33:43 +00:00

packages/app/src/Element/Text.tsx Outdated

						
				@ -17,9 +17,10 @@ export interface TextProps {

				  disableMedia?: boolean;

				  disableMediaSpotlight?: boolean;

				  depth?: number;

				  searchedValue?: string;

Kieran commented

2023-08-30 15:33:23 +00:00

props name should be highlighText accepting the RegExp

props name should be `highlighText` accepting the `RegExp`

fernandoporazzi marked this conversation as resolved

packages/app/src/Element/Text.tsx Outdated

						
				@ -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={{

Kieran commented

2023-08-30 15:32:29 +00:00

This is an obvious XSS leak, dangerouslySetInnerHTML should NEVER be used

This is an obvious XSS leak, `dangerouslySetInnerHTML` should NEVER be used

fernandoporazzi commented

2023-08-30 15:42:13 +00:00

What would you suggest as an alternative?

A lib like DOMPurify could get the job done here.

What would you suggest as an alternative? A lib like [DOMPurify](https://www.npmjs.com/package/dompurify) could get the job done here.

Kieran commented

2023-08-30 17:28:01 +00:00

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 styles

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 styles

fernandoporazzi commented

2023-08-31 10:27:42 +00:00

I implemented the changes using the transformText approach. Now there is a new type for the ParsedFragment interface.

I implemented the changes using the `transformText` approach. Now there is a new type for the `ParsedFragment` interface.

fernandoporazzi marked this conversation as resolved

fernandoporazzi force-pushed highlight-searched-word from 4a5e32d4d4 to a6d5aa9453

2023-08-31 10:25:37 +00:00

Compare

fernandoporazzi force-pushed highlight-searched-word from a6d5aa9453 to b318a102b6

2023-08-31 10:30:12 +00:00

Compare

fernandoporazzi force-pushed highlight-searched-word from b318a102b6 to 501f8694c5

2023-08-31 10:31:01 +00:00

Compare

fernandoporazzi commented

2023-08-31 10:41:15 +00:00

New implementation submitted. Preview:

Screenshot 2023-08-31 at 12.22.52.png

317 KiB

Screenshot 2023-08-31 at 12.40.42.png

160 KiB

Kieran requested changes 2023-08-31 11:16:51 +00:00

packages/app/src/Element/Text.tsx Outdated

						
				@ -25,3 +27,3 @@

				  const elements = useMemo(() => {

				    return transformText(content, tags);

				    return transformText(content, tags, highlighText);

Kieran commented

2023-08-31 11:14:52 +00:00

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 this useMemo

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 this `useMemo`

fernandoporazzi commented

2023-08-31 11:39:16 +00:00

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 and highlightText into a single object and make the highlightText an optional property.

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` and `highlightText` into a single object and make the `highlightText` an optional property.

Kieran commented

2023-08-31 12:16:03 +00:00

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 outside

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 outside

fernandoporazzi commented

2023-09-01 09:15:22 +00:00

I tried to co-locate the function extractHighlightedText inside this useMemo, 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 the transformText, we'd have to have the implementation on top of the type text fragment directly on the render. I don't know if it's clear what I mean 😅

Something like:

default:
  return (
    <div className="text-frag">
      {highlighText ? applyHighlight(a.content) : a.content}
    </div>
  );

That way we remove the highlightText param from the function and fully delegate this to the Text component.

Opinions?

I tried to co-locate the function `extractHighlightedText` inside this `useMemo`, 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 the `transformText`, we'd have to have the implementation on top of the type `text` fragment directly on the render. I don't know if it's clear what I mean 😅 Something like: ```tsx default: return ( <div className="text-frag"> {highlighText ? applyHighlight(a.content) : a.content} </div> ); ``` That way we remove the highlightText param from the function and fully delegate this to the Text component. Opinions?

Kieran commented

2023-09-01 09:38:18 +00:00

Yea thats probably fine

fernandoporazzi commented

2023-09-01 13:15:54 +00:00

Implemented and PR is updated!