-
Notifications
You must be signed in to change notification settings - Fork 84
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 component #64
base: master
Are you sure you want to change the base?
Search component #64
Conversation
<S.IconWrapper> | ||
<SearchIcon aria-label="Search" /> | ||
</S.IconWrapper> | ||
<Search /> |
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.
Since we added the search here, we'll need to mock inside tests to not break the unit tests.
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.
Great point! I'll add the mock and push changes.
import * as S from './styles' | ||
import Button from 'components/Button' | ||
|
||
const Search = () => { |
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'd separate those parts in different components (maybe even internal ones) like:
- Search
- SearchBox
- Hits
- NoResults
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.
Agree. I'll break it up into smaller components as you suggest
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.
hey @willianjusten just to check before pushing changes, do you think this structure is valid/makes sense?
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.
Yeah, that was exactly what I thought. Since they're dependant, doesn't make sense to have it separated but together like this.
I made the changes restructuring the component as you suggested and added the mock for the menu. I remade the tests for each component, but got stuck on how to mock higher order components, specifically the connectors at |
Description
It adds a search bar at the menu, a hidden input which opens on click taking all available space. It uses Algolia's react-instantsearch package to create UI elements.
Requirements
NEXT_PUBLIC_MEILISEARCH_SERVER
,NEXT_PUBLIC_MEILISEARCH_PUBLIC_KEY
Screenshots
Button fix
It is also added
white-space: nowrap
to the button component to prevent line breaks (otherwise appearance is modified) :I would love to hear your feedback on it, being my main concerns if the component is too large (and if would be a good idea to break it up into smaller components) and the tests part. Open to modify anything you think appropiate =)