-
Notifications
You must be signed in to change notification settings - Fork 238
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: bookmark search #638
Conversation
Made a start on the search functionality
…r the search element
Improved search query, suggested bookmark query, and working unit test for basics
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.
Just had some minor concerns, but great progress here! 🎉
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.
Almost there, one minor request. Btw, have you confirmed that everything works locally?
const feedProps = useMemo<FeedProps<unknown>>(() => { | ||
if (isSearchOn && searchQuery) { | ||
return { | ||
feedQueryKey: ['bookmarks', user?.id ?? 'anonymous', searchQuery], |
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.
Using the bookmarks while not logging in should not be possible.
If the user is not logged in maybe we should disable the 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.
You actually can't get the bookmarks feed if not logged in.
The BookmarkFeedPage will redirect you if the token is refreshed.
I assumed this was a second fallback in the split second.
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.
Oh and fully tested locally yes
Waiting for @sshanzel to approve his ongoing PR and you can squash and merge. Just make to keep the commit convention :) |
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 very well to me! 🚢
This PR is to refactor the existing bookmark page
In the new version:
Bookmark page now has it's own layout system
Bookmark has a search menu
Bookmark has it's own search page (based on the layout)
Search adjusted to be more dynamic searchable (based on feed type)
Should be checked together with the PR on the API side.
DD-78 #done
Edit: Added the test cases