-
Notifications
You must be signed in to change notification settings - Fork 0
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
Newswires UI: add pagination #108
Conversation
9f268bf
to
3fad1a0
Compare
3fad1a0
to
54a1357
Compare
54a1357
to
6b19f70
Compare
@@ -167,15 +180,34 @@ export function SearchContextProvider({ children }: PropsWithChildren) { | |||
}); | |||
} | |||
|
|||
if (state.status === 'success' || state.status === 'offline') { | |||
if (state.status === 'loading-more') { | |||
fetchResults(currentConfig.query, { |
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 wonder whether it would be more straightforward to put the fetch().then().catch()
into the body of the loadMoreResults
function? It will hopefully keep the number of branches in the status
state machine simpler, and as far as I can tell it's only the load more button itself that needs to know about whether the fetch is underway, so we could perhaps have 'loading-more' as a piece of more local state. What do you think?
b856e3d
to
7f1c1e0
Compare
2aec945
to
a5c5ad9
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.
this looks great to me! Added a couple of small suggestions but not blocking 👍
a5c5ad9
to
0fe356d
Compare
What does this change?
Add a load more button to view older stories.
How to test
How can we measure success?
Have we considered potential risks?
Images
Accessibility