-
Notifications
You must be signed in to change notification settings - Fork 816
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
feat: add document search to the command menu #713
feat: add document search to the command menu #713
Conversation
@ksushant6566 is attempting to deploy a commit to the Documenso Team Team on Vercel. A member of the Team first needs to authorize it. |
Important Auto Review SkippedAuto reviews are limited to the following labels: coderabbit. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the TipsChat with CodeRabbit Bot (
|
{ label: string; path: string; recipents: string[] }[] | ||
>([]); | ||
|
||
const { isLoading: isSearchingDocuments, refetch: findDocuments } = |
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.
Why not use the data
property instead? Using onSuccess on queries will be deprecated with React Query 5 afaik
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.
makes sense, I'll make change it
filter: (value, search) => { | ||
const exists = value.includes(search); | ||
if (exists) return 1; | ||
|
||
const result = searchResults.find( | ||
(result) => result.label.toLocaleLowerCase() === value.toLocaleLowerCase(), | ||
); | ||
|
||
if (result?.recipents.some((recipent) => recipent.includes(search))) return 1; | ||
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.
Wonder if we can cheat and just use an sr-only
thing that contains the recipients so we don't have to add a custom filter fn 🤔
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.
not sure what you mean by sr-only? can you please explain?
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.
added a new value
property in searchResults
const newResults = searchDocuments?.map((document) => ({
label: document.title,
path: `/documents/${document.id}`,
value: document.title + document.Recipient.map((recipient) => recipient.email).join(' '),
}));
with this we don't need a custom filter function anymore
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.
Sorry I meant the tailwind sr-only
class that would make text invisible unless on a screen reader 😄
If the solution above works that's just as good!
where: { | ||
OR: [ | ||
{ | ||
title: { | ||
contains: query, | ||
mode: 'insensitive', | ||
}, | ||
}, | ||
{ | ||
Recipient: { | ||
some: { | ||
email: { | ||
contains: 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.
This is currently missing documents you have received, see findDocuments
for some inspo on how to query it.
Consider if we could also use findDocuments
instead with some modifications
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.
cools, I'll check and make the necessary changes
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.
although the findDocuments
does have a filter by term
but searching term
in recipients email might be a lil confusing & unintuitive for others so I just made the changes in SearchDocumentsWithKeyword
to include received documents as well.
…ctor to remove custom filter
data: searchDocuments, | ||
isLoading: isSearchingDocuments, | ||
refetch: findDocuments, | ||
} = trpcReact.document.searchDocuments.useQuery( |
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.
Ideally we always search and show the most recent documents
|
||
useEffect(() => { | ||
if (search) { | ||
void findDocuments(); |
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 is redundant since we're using react query it should just automatically fetch whenever query changes
|
||
useEffect(() => { | ||
if (searchDocuments) { | ||
const newResults = searchDocuments?.map((document) => ({ |
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 could be done with a map of the results in a use memo or similar, an effect will introduce race conditions
const { query } = input; | ||
|
||
try { | ||
const documents = await SearchDocumentsWithKeyword({ |
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.
We don't do pascal case methods :)
// }, [searchDocuments]); | ||
|
||
const searchResults = useMemo(() => { | ||
if (!searchDocuments) return []; |
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.
No single line if statements please, just wrap it in braces {}
😄
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.
done
}, | ||
}); | ||
|
||
const documents = await prisma.document.findMany({ |
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.
Looks like we're missing a limit here, if I were to search for a
I might return thousands of 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.
Ideally this would be an option passed into the method
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.
done now this takes a limit
variable with a default value of 5.
query: search, | ||
}, | ||
{ | ||
enabled: search !== '', |
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.
We still want results even for an empty search term 😄
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.
cool, done!
@@ -83,3 +83,7 @@ export const ZDeleteDraftDocumentMutationSchema = z.object({ | |||
}); | |||
|
|||
export type TDeleteDraftDocumentMutationSchema = z.infer<typeof ZDeleteDraftDocumentMutationSchema>; | |||
|
|||
export const ZSearchDocumentsMutationSchema = z.object({ | |||
query: z.string().min(1), |
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.
Which will meant his also needs to change as well
Hey @Mythie , just pushed requested changes. if (anything needs to be changed) { please let me know :) } |
round 2 |
It's because of migrations aha! This PR has been built on top of the other PR for document deletion so it won't work on a fresh checkout like the one I did on Gitpod |
Yes! |
As a final request then I'll need a video recording showing the 3 flows:
Once this has been provided I'm happy to accept this submission 🙌 |
cool give me 5 minutes |
Hey @Mythie , here is the loom video of the final solution https://www.loom.com/share/3593cfad580f48ca8d7badb24ab776d8?sid=3286d03b-199a-489a-8ab5-8135de6de241 |
Looks good! I’m happy with it in this state and the code is alright too 🙌🏻 You only lose points for being a dark mode user 🌚 I’ll merge once I know it won’t blow up |
Cool 💯 |
The cmdk library filters based on whether the item contains a string sequence of search in order, but not necessarily right next to each other, but the filter you have implemented filters whether the items containing search substring right next to each other This feature can be implemented with regex, if you want I can build it. |
resolves #702