-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐶 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the issue: Joystream/joystream#2608
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd feel safer with some tests for the highlight component :)
shorten?: boolean | ||
children: string | ||
} | ||
export const HighlightedText = memo(({ pattern, shorten, children }: HighlightedTextProps) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 getsC1
from the cacheC1 > C2 > C3 > thread2
: queriesC3
, and getsC2
andC1
from the cacheC1 > thread3
: queriesC1
WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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