Skip to content
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

Merged
merged 13 commits into from
Oct 11, 2021
Merged

Feat: bookmark search #638

merged 13 commits into from
Oct 11, 2021

Conversation

rebelchris
Copy link
Contributor

@rebelchris rebelchris commented Oct 7, 2021

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

@rebelchris rebelchris removed the request for review from idoshamun October 7, 2021 14:31
Copy link
Member

@sshanzel sshanzel left a 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! 🎉

packages/shared/src/components/BookmarkFeedLayout.tsx Outdated Show resolved Hide resolved
packages/shared/src/components/BookmarkFeedLayout.tsx Outdated Show resolved Hide resolved
packages/shared/src/components/BookmarkFeedLayout.tsx Outdated Show resolved Hide resolved
packages/webapp/components/layouts/BookmarkFeedPage.tsx Outdated Show resolved Hide resolved
Copy link
Member

@idoshamun idoshamun left a 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],
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@idoshamun
Copy link
Member

Waiting for @sshanzel to approve his ongoing PR and you can squash and merge. Just make to keep the commit convention :)

Copy link
Member

@sshanzel sshanzel left a 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! 🚢

@rebelchris rebelchris merged commit 135e48e into master Oct 11, 2021
@rebelchris rebelchris deleted the DD-78-bookmark-search branch October 11, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants