-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
…vtex-sites/base.store into feat/FSSS-139-history-component
Co-authored-by: Lucas Feijó <lucasfjportela@gmail.com>
Co-authored-by: Israel Costa <israelswf@gmail.com>
✅ Deploy Preview for basestore ready! 🔨 Explore the source changes: 2ed3b8f 🔍 Inspect the deploy log: https://app.netlify.com/sites/basestore/deploys/6234d87e3b5a7800097ca0a4 😎 Browse the preview: https://deploy-preview-391--basestore.netlify.app |
Preview is readyThis pull request generated a Preview👀 Preview: https://preview-391--base.preview.vtex.app |
Gatsby Cloud Build Reportbasestore 🎉 Your build was successful! See the Deploy preview here. Build Details🕐 Build time: 11m PerformanceLighthouse report
|
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 job, Sara ✨
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.
Left some initial comments for us to discuss!
import useSearchHistory from 'src/sdk/search/useSeachHistory' | ||
import { formatSearchState, initSearchState } from '@faststore/sdk' | ||
|
||
const doSearch = (term: string) => { |
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.
Noticed we have this same function on the SearchInput
component so I wonder we can how we can extract it to a single place.
base.store/src/components/common/SearchInput/SearchInput.tsx
Lines 24 to 30 in 157ac94
const doSearch = async (term: string) => { | |
const { pathname, search } = formatSearchState( | |
initSearchState({ | |
term, | |
base: '/s', | |
}) | |
) |
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.
Nice catch!
I think when we have all search-related components integrated we are going to have a more clear way about how to reuse the same 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.
I agree with @eduardoformiga. How do you guys feel about adding a task to do that in the next sprint?
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 with you!
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.
Good Job! 👏
I left some comments/suggestions regarding the row alignment.
import useSearchHistory from 'src/sdk/search/useSeachHistory' | ||
import { formatSearchState, initSearchState } from '@faststore/sdk' | ||
|
||
const doSearch = (term: string) => { |
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.
Nice catch!
I think when we have all search-related components integrated we are going to have a more clear way about how to reuse the same function.
Co-Authored-By: Sara Nicoly <saranicoly0@gmail.com>
Co-Authored-By: Sara Nicoly <saranicoly0@gmail.com>
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 job, LGTM! 💯
Just a small thing I noticed is that the CSS file name is not following the pattern we have in the project. Instead using SearchHistory.scss
you can leave as search-history.scss
to keep consistency.
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.
LGTM! Just one more question about opening the searches in a new tab.
…o feat/FSSS-193-history-component
Co-authored-by: Filipe W. Lima <fwl.ufpe@gmail.com>
What's the purpose of this pull request?
This PR aims to show the user's search history in base store, which is made by creating a "SearchHistory" component.
How it should look like:


How it is actually looking like:
How does it work?
Gravacao.de.Tela.2022-03-15.as.19.35.16.mov
Gravacao.de.Tela.2022-03-15.as.19.52.24.mov
Gravacao.de.Tela.2022-03-15.as.20.07.35.mov
How to test it?
Since the component will be part of the Suggestion's dropdown component, we don't have a specific place to render it yet. That said, you can test SearchHistory by following the steps below:
Supposing that you want to see it on Navbar:
const { searchHistory, clearSearchHistory } = useSearchHistory()
inside the Navbar() function:Possible issues
References
Checklist