🐶 Implement the forum search results popup #1309
Conversation
| const byBestMatch = <T extends Record<any, any>>(search: string, fields: ((x: T) => string)[]) => { | ||
| const patterns = [RegExp(`\\b${search}\\b`, 'gi'), RegExp(search, 'gi')] | ||
|
|
||
| return (a: T, b: T): number => { | ||
| for (const field of fields) { | ||
| const fieldA = field(a) | ||
| const fieldB = field(b) | ||
| if (fieldA === fieldB) continue | ||
| for (const pattern of patterns) { | ||
| const matchA = fieldA.match(pattern)?.length ?? 0 | ||
| const matchB = fieldB.match(pattern)?.length ?? 0 | ||
|
|
||
| if (matchA !== matchB) return matchB - matchA | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
| } |
There was a problem hiding this comment.
This part should be done by the query node. I will open an issue for a text search query.
There was a problem hiding this comment.
Here's the issue: Joystream/joystream#2608
| shorten?: boolean | ||
| children: string | ||
| } | ||
| export const HighlightedText = memo(({ pattern, shorten, children }: HighlightedTextProps) => { |
There was a problem hiding this comment.
There's a lot of logic in the component. Could you write a test for it?
| children: string | ||
| } | ||
| export const HighlightedText = memo(({ pattern, shorten, children }: HighlightedTextProps) => { | ||
| const nodes = (pattern ? [...children.matchAll(pattern)] : []).reduceRight( |
There was a problem hiding this comment.
If I read this correctly when there's no pattern we don't do any matching and just use the existing nodes? If so, I'd do that explicitly to not do anything on an empty array.
Another question is for the pattern property – what is its use-case? I couldn't figure it out from the storybook so maybe the tests will help.
There was a problem hiding this comment.
About the pattern this is the RegExp used to find what to highlight. In this case it would only be a string with no special character but passing the RegExp rather than a string makes this component more reusable by allowing fancier RegExp. Also it allows to create the RegExp once and just pass it down by reference.
| // The goal of this hook is to utilize apollo cache to be faster than `useForumCategoryBreadcrumbs` | ||
| // when there are a lot of breadcrumbs to show, like on search results |
There was a problem hiding this comment.
Could you briefly explain why this behaves better with Apollo cache?
Also, can we have just one or does the previous one have other purposes as well?
There was a problem hiding this comment.
The previous useForumCategoryBreadcrumbs is much better for fetching a single breadcrumbs because it does it in only one query. But because there can many results to render with different breadcrumbs but which are probably sharing some root categories, I thought that for this particular case, it could be better to have slightly more but shorter cacheable queries.
Eg:
There are several results from C1 > C2 > thread1, from C1 > C2 > C3 > thread2, and from C1 > thread3
For their breadcrumbs:
C1 > C2 > thread1: queriesC2, and getsC1from the cacheC1 > C2 > C3 > thread2: queriesC3, and getsC2andC1from the cacheC1 > thread3: queriesC1
WDYT ?
There was a problem hiding this comment.
I think that the idea is pretty neat – thanks for the explanation. If we could discuss that earlier I would opt-in for using the existing hook and adding optimization if or when we'd saw a need for that. Just because it is hard to tell how real-world data will present. For instance, if most of the results are from a single category, then they should be cached already. OTOH it might be a case that most of the results came from single parents, but multiple sub-categories. Then, our solution will be better.
But since you already implemented this let's keep that and see what will happen.
ps.: Another solution could be to add breadcrumbs to the forum thread/category in Hydra itself as it is used in multiple places.
|
@jodator sorry I should have changed it into a draft, I kept finding some stuff I forgot about. I'll commit those in another PR soon |
Closes #1081