-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add reusable debounce hook to improve search query responsiveness #3137
base: main
Are you sure you want to change the base?
Conversation
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.
Now I can see the difference .. nice work
I didn't test native yet...
web/src/routes/SearchPage.tsx
Outdated
useEffect(() => { | ||
const appendToUrl = filterText.length !== 0 ? `?query=${filterText}` : '' | ||
navigate(`${pathname}/${appendToUrl}`, { replace: true }) | ||
}, [debouncedQuery, navigate, location.pathname]) |
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.
useEffect(() => { | |
const appendToUrl = filterText.length !== 0 ? `?query=${filterText}` : '' | |
navigate(`${pathname}/${appendToUrl}`, { replace: true }) | |
}, [debouncedQuery, navigate, location.pathname]) | |
useEffect(() => { | |
const appendToUrl = debouncedQuery.length !== 0 ? `?query=${debouncedQuery}` : '' | |
navigate(`${pathname}/${appendToUrl}`, { replace: true }) | |
}, [debouncedQuery]) |
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.
we need here navigate and pathname dependencies
Adding memo to SearchListItem will significantly make a difference from my observation because it will skip re-rendering when props are changed. memo
|
4041d1d
to
fa5464c
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.
Tested on web, nice idea @bahaaTuffaha and well done @lunars97!
This is closely related and might conflict with #3117 if my proposals are implemented. In fact, #3117 is actually caused imo by the duplication of state in the url search params and filter text use state.
@@ -49,8 +50,10 @@ const SearchModal = ({ | |||
const resourceCache = useResourceCache({ cityCode, languageCode }) | |||
const { t } = useTranslation('search') | |||
|
|||
const contentLanguageResults = useSearch(documents, query) | |||
const fallbackLanguageResults = useSearch(fallbackLanguageDocuments, query) | |||
const debouncedQuery = useDebounce(query, DEBOUNCED_QUERY_TIMEOUT) |
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 think it would be nice to use DEBOUNCED_QUERY_TIMEOUT
as default, such that there is no need to import/explicitly pass this here.
const debouncedQuery = useDebounce(query, DEBOUNCED_QUERY_TIMEOUT) | |
const debouncedQuery = useDebounce(query) |
@@ -0,0 +1,17 @@ | |||
import { useState, useEffect } from 'react' | |||
|
|||
const useDebounce = <T>(value: T, delay: number) => { |
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 think it would be nice to set a default here. As long as it is only used here, there is no need to keep it in constants
.
const useDebounce = <T>(value: T, delay: number) => { | |
const DEBOUNCE_TIMEOUT = 250 | |
const useDebounce = <T>(value: T, delay = DEBOUNCE_TIMEOUT) => { |
const debouncedValueTimeout = setTimeout(() => { | ||
setDebouncedValue(value) | ||
}, delay) |
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.
🙃 Optional:
const debouncedValueTimeout = setTimeout(() => { | |
setDebouncedValue(value) | |
}, delay) | |
const debouncedValueTimeout = setTimeout(() => setDebouncedValue(value), delay) |
@@ -38,6 +39,13 @@ const SearchPage = ({ city, cityCode, languageCode, pathname }: CityRouteProps): | |||
const navigate = useNavigate() | |||
const fallbackLanguage = config.sourceLanguage | |||
|
|||
const debouncedQuery = useDebounce(filterText, DEBOUNCED_QUERY_TIMEOUT) |
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.
const debouncedQuery = useDebounce(filterText, DEBOUNCED_QUERY_TIMEOUT) | |
const debouncedQuery = useDebounce(filterText) |
useEffect(() => { | ||
const appendToUrl = debouncedQuery.length !== 0 ? `?query=${debouncedQuery}` : '' | ||
navigate(`${pathname}/${appendToUrl}`, { replace: true }) | ||
}, [debouncedQuery, navigate, pathname]) |
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.
❓ Is there a reason why we should not directly update the url? I think the search input and the url should always be in sync to avoid weird side effects. I think its okay that the url is updated before the search results are.
Edit: I see, problems arise due to the use of a useState
for filterText
. I think we should remove it and only use/update the query in the url search params directly like this by replacing L36 and L37
const query = new URLSearchParams(useLocation().search).get('query') ?? ''
const [filterText, setFilterText] = useState<string>(query)
with
const [searchParams, setSearchParams] = useSearchParams()
const query = searchParams.get('query') ?? ''
It is always desirable to avoid duplicated states, in this case the url search params are enough. Obviously, this was not introduced in your PR but I think it would be cool if you could refactor this here accordingly?
Let me know if you have any questions :)
For more information on the useSearchParams
hook see https://api.reactrouter.com/v7/functions/react_router.useSearchParams.html
setFilterText(filterText) | ||
const appendToUrl = filterText.length !== 0 ? `?query=${filterText}` : '' | ||
navigate(`${pathname}/${appendToUrl}`, { replace: true }) |
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.
🔧 As mentioned above, I think we should directly update the url, so this should probably be reverted. With the change from above, you could then do the following here:
setSearchParams({ query })
const appendToUrl = query.length !== 0 ? `?query=${query}` : ''
navigate(`${pathname}/${appendToUrl}`, { replace: true })
Short Description
This is an improved version of already merged PR #3103. Thanks to @bahaaTuffaha who noticed that the responsiveness has not been that much improved and suggested to apply debounce function directly in SearchPage.tsx (web) and SearchModal.tsx (native). I also noticed that the responsiveness has become much better.
Proposed Changes
useDebounce
in sharedSide Effects
should be none
Resolved Issues
Fixes: #