-
Notifications
You must be signed in to change notification settings - Fork 188
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
[next] Issue 1500 - Port SearchInput to NEXT.js #1580
Conversation
I had to import this component in another PR. Please review: #1581 Because there you will find the searchBar and the SearchInput. |
|
||
type searchInputProps = { | ||
text: string; | ||
onTextChange: Function; |
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.
same question here as in #1581 , should we be more specific about the type here? Maybe EventHandler
instead of Function
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.
It's an onChange event so ChangeEvent is the type here, same as in #1581
Fix that, and this LGTM
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 do it in a follow-up PR, because we are drilling a lot of stuff here. After getting everything working we will need to create an interface to SearchPage
's components and a context.
These functions are connected to how we build the URL query of a search. So I believe that we need to be care full with that. It's working now. Let's keep this for now.
I'm opening an issue for the next step (interface and context) now.
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 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.
Sounds good
Hi Pedro, please see #1545 (comment) |
Issue This PR Addresses
Fixes #1500
Type of Change
Description
Port SearchInput to NEXT.js
Checklist