-
Notifications
You must be signed in to change notification settings - Fork 0
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 fuse #91
Implement fuse #91
Conversation
implemented with useEffect, looking for alternative solution
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
onika burgers
@@ -22,6 +22,9 @@ export default function Roles({ | |||
searchParams, | |||
}: { | |||
searchParams?: { | |||
workTerm?: WorkTermType; | |||
workEnvironment?: WorkEnvironmentType; | |||
search?: string; | |||
cycle?: WorkTermType; |
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.
it looks like we want to get rid of cycle
and term
now since you renamed them in the new search params.
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.
Yeah I think we got rid of workTerm
and workEnvironment
in favor of cycle
and term
but I might be wrong. This might an artifact of a merge. @suxls would recommend removing workTerm
and workEnvironment
from the searchParams
object and testing.
options: validationResult.success ? validationResult.data : {}, | ||
}); | ||
|
||
const [selectedReview, setSelectedReview] = useState<ReviewType | undefined>( | ||
reviews.data ? reviews.data[0] : undefined, | ||
); | ||
|
||
useEffect(() => { |
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.
in general we want to avoid this kind of useEffect
, but I'm assuming there was some kind of bug due to the fact that reviews are updated based on the child SearchFilter
component?
@@ -16,11 +16,15 @@ const formSchema = z.object({ | |||
|
|||
export type SearchFilterFormType = typeof formSchema; | |||
|
|||
export default function SearchFilter() { | |||
interface SearchFilterProps { |
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 know this is so my inner fundies 1 ta but what are our thoughts on starting to us js doc to lightly document the functions. Espesh since dev turnover may be on the rise (oml we are getting old). Up to rishi hes TL i'm just throwing this out there ❤️ 🙈
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 agree with this to make it easier for everyone to read
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.
YES pls document with JSDoc 🙏🏻 I remember we were doing it for a bit at the start of Spring but that fizzled out lol.
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.
yessir
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're almost there 📣
@@ -22,6 +22,9 @@ export default function Roles({ | |||
searchParams, | |||
}: { | |||
searchParams?: { | |||
workTerm?: WorkTermType; | |||
workEnvironment?: WorkEnvironmentType; | |||
search?: string; | |||
cycle?: WorkTermType; |
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.
Yeah I think we got rid of workTerm
and workEnvironment
in favor of cycle
and term
but I might be wrong. This might an artifact of a merge. @suxls would recommend removing workTerm
and workEnvironment
from the searchParams
object and testing.
@@ -63,16 +66,23 @@ export default function Roles({ | |||
}, [toast, mounted]); |
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.
You'll have to update the dependency array to include the validationResult
based on that warning.
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
@@ -16,11 +16,15 @@ const formSchema = z.object({ | |||
|
|||
export type SearchFilterFormType = typeof formSchema; | |||
|
|||
export default function SearchFilter() { | |||
interface SearchFilterProps { |
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.
YES pls document with JSDoc 🙏🏻 I remember we were doing it for a bit at the start of Spring but that fizzled out lol.
return reviews; | ||
} | ||
|
||
const fuseOptions = { |
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.
Can we maybe abstract the fuzzy searching out to a helper? I might implement a custom linting rule that makes sure that every function should be 4 or less lines of code 😡 /s
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.
will implement that on a separate branch
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.
Comments left in the files - I agree with Rishi's note that the workTerm and workEnvironment should be removed
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.
LGTM
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.
blind approve
Description
Incorporated Fuse.js to the searching process.
Motivation and Context
Added fuse.js at the endpoint of the project.
Currently, the fuse is using the review fields such as "reviewHeadline", "textReview", "location", this can be modified later base on what we want to prioritize. With that said, I'm working on abstracting out fuse as an util object where different search bar will use fuse that prioritize different fields.
A bug on the side, an useEffect is added to fix the bug where if the page don't refresh, the displayed review is not updated to the first one of the resulted search list.
Closes #76
Closes #75
How has this been tested?
haha testing
Screenshots (if appropriate):
Types of changes
Checklist: