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

Search custom feeds #1031

Merged
merged 7 commits into from
Jul 28, 2023
Merged

Search custom feeds #1031

merged 7 commits into from
Jul 28, 2023

Conversation

ansh
Copy link
Contributor

@ansh ansh commented Jul 18, 2023

Allows you to search custom feeds by their name or description

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. I think we could squeeze slightly better perf out of it by moving the feeds property to _feeds and replacing it with a get feeds() accessor that runs the filtering logic against _feeds that's currently being applied in search(). Then the search function would just set a a searchQuery property that drives the get feeds() accessor, and you can drop some of the refreshes in the view when the search query state changes.

@pfrazee
Copy link
Collaborator

pfrazee commented Jul 18, 2023

Oh I accidentally marked that as approved instead of just a comment. Sec while I test.

@ansh
Copy link
Contributor Author

ansh commented Jul 18, 2023 via email

@ansh ansh marked this pull request as draft July 18, 2023 19:58
@ansh
Copy link
Contributor Author

ansh commented Jul 19, 2023

Waiting on confirmation from @estrattonbailey about implementing this on the backend... Steady lads 🖖🏽

@ansh ansh marked this pull request as ready for review July 26, 2023 20:43
@ansh
Copy link
Contributor Author

ansh commented Jul 26, 2023

@pfrazee Need a re-review since now search is taking place on the backend

@pfrazee
Copy link
Collaborator

pfrazee commented Jul 27, 2023

@ansh What's the status of this being deployed on backend? I just tested and it's not in prod yet

@pfrazee
Copy link
Collaborator

pfrazee commented Jul 27, 2023

Scratch that -- the validation was failing clientside because I hadnt run yarn

src/view/screens/DiscoverFeeds.tsx Outdated Show resolved Hide resolved
@ansh ansh requested a review from pfrazee July 28, 2023 01:39
const [query, setQuery] = React.useState<string>('')
const debouncedSearchFeeds = React.useMemo(
() => debounce(() => feeds.search(query), 200), // debouce for 200 ms
[feeds, query],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't working -- passing query into the memo deps was recreating the function every time, losing the debounce state and having no effect.

I fixed that, tuned the timeout to 1s, and then updated onSubmitQuery to use debounce but run a flush() so that it triggers immediately

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@pfrazee pfrazee merged commit 38d78e1 into main Jul 28, 2023
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.

2 participants