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

Add pagination to Shopify integration #391

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

brookewp
Copy link
Contributor

This implements the pagination feature (@chriszarate added in #338) to our Shopify integration.

To get the total product count, we'd need access to read_products which requires authenticating with OAuth. To avoid this, I've made total_items optional and added has_next_page, so we can paginate through products one page at a time:

Screen.Recording.2025-02-24.at.5.49.25.PM.mov

I also exposed setPerPage so the items per page can be updated, which makes going through the products a lot faster. This will also update for any sources with pagination set up (e.g. the art institute).

@@ -157,7 +162,7 @@ export function ItemList( props: ItemListProps ) {
onChangeView={ onChangeView }
paginationInfo={ {
totalItems: totalItems ?? data.length,
totalPages: totalPages ?? 1,
totalPages: totalPages ?? ( hasNextPage ? page + 1 : page - 1 ) ?? 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't quite work. The part in parentheses will always return a non-null value, so the 1 is never reached

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. What do you think of using Math.max?

totalPages: totalPages ?? ( hasNextPage ? page + 1 : Math.max( 1, page ) ),

@@ -88,6 +89,7 @@ export function usePaginationVariables( {
supportsCursorPagination || supportsPagePagination || supportsOffsetPagination;
const totalItems = paginationData?.totalItems;
const totalPages = totalItems && perPage ? Math.ceil( totalItems / perPage ) : undefined;
const hasNextPage = paginationData?.hasNextPage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you update the supports*Pagination variables above to take into account the statement we made in the schema? "either total_items or has_next_page must be defined"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this?

	const supportsPagination =
		supportsCursorPagination ||
		supportsPagePagination ||
		supportsOffsetPagination ||
		totalItems ||
		hasNextPage;

Signed-off-by: brookewp <[email protected]>
@brookewp brookewp requested a review from chriszarate February 27, 2025 21:18
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