-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #3522] refine fix for pagination showing with no results #3533
base: main
Are you sure you want to change the base?
Conversation
@@ -52,11 +45,13 @@ export default function SearchSortBy({ | |||
}, | |||
]; | |||
|
|||
const handleChange = (event: React.ChangeEvent<HTMLSelectElement>) => { | |||
const newValue = event.target.value; | |||
updateTotalResults(totalResults); |
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's no need to set total results in multiple places in the app, since both of these components are responding to the same request
|
||
const updatePage = (page: number) => { | ||
updateTotalPages(String(total)); | ||
updateTotalResults(totalResults); |
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.
with these functions only being called in updatePage
the client side pagination state was only being updated on pagination clicks, not in response to the search results updating from the completed API request. This is why the pagination was not responding to adding filters the way we expected, resulting in the pagination still sometimes showing up even if there were no search results. With this new setup, the state will be set purely based on the results of the search request whenever a new request is made
@@ -3,7 +3,7 @@ | |||
import { useSearchParams } from "next/navigation"; | |||
import { createContext, useCallback, useMemo, useState } from "react"; | |||
|
|||
interface QueryContextParams { | |||
export interface QueryContextParams { |
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 to export this? Didn't see it called elsewhere
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 catch, this is left over from a previous idea. Removing.
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.
👍🏿
fd43acc
to
621edc4
Compare
Summary
Fixes #3522
Time to review: 15 mins
Changes proposed
While the client facing issue for the bug mentioned in the ticket was fixed in #3524 this provides a more comprehensive fix by correcting some slightly funky behavior in pagination related components
Context for reviewers
Additional information