-
Notifications
You must be signed in to change notification settings - Fork 21
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(sort-filter): search project by name #533
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
You can use |
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.
Thanks a lot! Just have a comment
options={["name_asc", "name_desc"]} | ||
value={`${orderBy}_${sortOrder}`} | ||
onChange={async (sort) => { | ||
const [order, sorting] = sort.split("_") as [OrderBy, SortOrder]; | ||
|
||
await setFilter({ orderBy: order, sortOrder: sorting }).catch(); | ||
}} | ||
onChange={onChangeSortByDropdown} |
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.
Do you think we should have all the sortLabels
back? including the time_asc
and time_desc
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 just moved the onChangeSortByDropdown
code above to use it with useCallback
to make it a bit more efficient 😊
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 added the |
You still need to use |
@kittybest @0xmad hi guys! 😊 I added the fixes for your current comments and also pushed my housekeeping fixes to prevent console errors. Could you review the file changes? I added a comment to each file to explain a bit more what I was doing. After your final approval I will squash the commits to merge this PR Thank you for your help :) |
aebbbdf
to
ccfc38e
Compare
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.
@NicoSerranoP thanks, just some changes requested
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
ref: PolymorphicRef<ElementType>, |
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 you don't use this ref, you don't need to add it as second param
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 tried but I am getting a console log error saying that forwardRef
needs a function that receives two parameters and it is not possible to remove the second one (ref). What I did is change its name to _
and removed the types
control={control} | ||
name={name} | ||
render={({ field: { value, onChange, ...field } }) => ( | ||
// eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events |
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.
div shouldn't have onClick
handlers.
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 div contains the upload image button and the div icon. I tried to change <div>
to <button>
but it didn't work (we cannot have nested buttons)
e80fe6e
to
48e80f0
Compare
feat(sort-filter): get projects with metadata from server feat(sort-filter): search by name on the server fix(frontend): fix small react bugs fix(frontend): fix favicon.svg call in html meta fix(main): view projects button as router and not link fix(form): prevent undefined url call fix(frontend): activate ssr to prevent hydration errors fix(search): useDeferredValue to lag behind on search fix(image): prevent console.log error fix(image): pr comments fix(search): single if
48e80f0
to
d6b0f56
Compare
|
||
/** | ||
* Fetch all approved projects with metadata | ||
* @param registryAddress |
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 another param called 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.
Thanks for also the housekeeping! But maybe split the housekeeping part in another PR better in the future? make single PR only focus on single stuff.
For this PR is totally fine :)
Description
I am activating the new feature to search projects by name using the search bar. Behind the scenes what I am doing is I am taking the
registryAddress
and the search bar input and sending it to the server to perform a search. The server fetches all the metadata objects and add them to the projects. Later on it filters them comparing thesearch
value and themetadata.name
value.Considerations
This current implementation performs the search on the server because it avoids problems with React
<InfiniteLoading>
anduseinfiniteQuery
in the client side. These two use pagination for better data efficiency and I did not want to break that feature.Changes made
@ctrlc03 @kittybest @crisgarner @0xmad what do you think? 😊