-
Notifications
You must be signed in to change notification settings - Fork 3
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: feeds + search page #474
Conversation
- make data type checkboxes work - make search limit + load more button work - tie search bar to URL query param
Preview Firebase Hosting URL: https://mobility-feeds-dev--pr-474-hqrwqt1t.web.app |
return () => { | ||
clearTimeout(timer); | ||
}; | ||
}, [searchLimit, searchParams, selectedFeedTypes]); |
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.
Let's add here the login event status. This can remove the need of the timer. Example code:
}, [isAuthenticatedOrAnonymous]); |
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 see you added this in main and I merged it in. The timer is needed to prevent search API calls on each keystroke, but this is probably better done by debouncing which I can add.
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.
The isAuthenticatedOrAnonymous is needed to avoid calls to the API that returned 403 as no users/anonymous are set at initial page rendering. I got the idea of the timer now, thanks. We were thinking of a different user interaction, but the design will be available for review later this week.
Another usability issue found
expected behavior: If there is no search values, there should be no displayed search results |
Stefan Philip seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
7be2a71
to
2e9358f
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.
looks good!
@stefan-philip would you be able to sign the CLA :) |
Summary:
This is to close #348.
Navigate to
/feeds
and type your search query in the search bar.