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 component #64

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

GuilleAngulo
Copy link
Contributor

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

  • Deploy MeiliSearch server.
  • Add strapi-plugin-meilisearch to the backend. Configure with server credentials and games collection.
  • Add env variables to the frontend: NEXT_PUBLIC_MEILISEARCH_SERVER, NEXT_PUBLIC_MEILISEARCH_PUBLIC_KEY

Screenshots

Desktop Mobile
desktop mobile

Button fix

It is also added white-space: nowrap to the button component to prevent line breaks (otherwise appearance is modified) :

button

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 =)

<S.IconWrapper>
<SearchIcon aria-label="Search" />
</S.IconWrapper>
<Search />
Copy link
Member

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.

Copy link
Contributor Author

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 = () => {
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

shot_210630_124245

Copy link
Member

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.

@GuilleAngulo
Copy link
Contributor Author

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 Hits/test. The problem I had is how to mock query and hits props for each test (I couldn't get it to work using spyOn ). I made an implementation (and is passing) but probably is not the right way to do it. Can you give me a hand with it? How could I mock in a correct way these connectors (connectStateResults, connectHits) and change the exposed props (query, hits)?

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