-
Notifications
You must be signed in to change notification settings - Fork 292
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 #755
Add Pagination #755
Conversation
I added a more docs-like RFC to the PR, copied below for easy-reading. PaginationPagination is a complex component, that becomes even more complex for online storefronts. The goal of our Pagination component should be to take on the undifferentiated difficult parts of paginating a Storefront APO collection in Hydrogen projects. This includes:
UsageCreate routeAdd a touch routes/products.tsx Fetch a Storefront connection in the loaderAdd a loader and query for the products in the shop. This is what a typical loader might look like without pagination applied. export async function loader({context, request}: LoaderArgs) {
const {products} = await context.storefront.query<{
products: ProductConnection;
}>(PRODUCTS_QUERY, {
variables: {
country: context.storefront.i18n?.country,
language: context.storefront.i18n?.language,
},
});
if (!products) {
throw new Response(null, {status: 404});
}
return json({products});
} And a sample query: const PRODUCTS_QUERY = `#graphql
query (
$country: CountryCode
$language: LanguageCode
) @inContext(country: $country, language: $language) {
products() {
nodes {
id
title
publishedAt
handle
variants(first: 1) {
nodes {
id
image {
url
altText
width
height
}
}
}
}
}
}
`; Add the pagination variables to the queryFirst import and use a helper + import {getPaginationVariables, PAGINATION_PAGE_INFO_FRAGMENT} from '~/components';
export async function loader({context, request}: LoaderArgs) {
const variables = getPaginationVariables(request, 4);
const {products} = await context.storefront.query<{
products: ProductConnection;
}>(PRODUCTS_QUERY, {
variables: {
+ ...variables,
country: context.storefront.i18n?.country,
language: context.storefront.i18n?.language,
},
});
if (!products) {
throw new Response(null, {status: 404});
}
return json({products});
} And a add the fragment and variables to the query: const PRODUCTS_QUERY = `#graphql
+ ${PAGINATION_PAGE_INFO_FRAGMENT}
query (
$country: CountryCode
$language: LanguageCode
+ $first: Int
+ $last: Int
+ $startCursor: String
+ $endCursor: String
) @inContext(country: $country, language: $language) {
products(
+ first: $first,
+ last: $last,
+ before: $startCursor,
+ after: $endCursor
) {
nodes {
id
title
publishedAt
handle
variants(first: 1) {
nodes {
id
image {
url
altText
width
height
}
}
}
}
+ pageInfo {
+ ...PaginationPageInfoFragment
+ }
}
}
`; Render the
|
An issue to consider as we move through build: #596 |
I feel + import {PAGINATION_PAGE_INFO_FRAGMENT} from '~/components';
const PRODUCTS_QUERY = `#graphql
+ ${PAGINATION_PAGE_INFO_FRAGMENT}
query (... ) {
products(... ) {
...
+ pageInfo {
+ ...PaginationPageInfoFragment
+ }
}
}
`; vs const PRODUCTS_QUERY = `#graphql
query (... ) {
products(... ) {
...
+ pageInfo {
+ hasPreviousPage
+ hasNextPage
+ startCursor
+ endCursor
+ }
}
}
`;
Example: I want to navigate between paginations like Dawn https://theme-dawn-demo.myshopify.com/collections/bags?page=2 and I also want to have infinite scroll. Definitely +1 on the abstracting away the previous/next Other questions:
|
I think this is a great proof of concept. A few thoughts and questions:
|
Here is an example of semi infinite pagination implementation: https://www.fashionnova.com/pages/search-results/clothing?page=3
|
I actually quite like how Ikea does it https://www.ikea.com/ca/en/search/?q=table Not sure how it is done but doing a page refresh does not lose the set of results you have already loaded - including if you click to a product and click back on the browser but I don't like for it not having a |
* @param autoLoadOnScroll enable auto loading | ||
* @param inView trigger element is in viewport | ||
* @param isIdle page transition is idle | ||
* @param connection Storefront API connection |
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.
These docs don't seem to match the actual signature? (Not that I'm diving into TS docs right now, I was more trying to understand what this function was doing)
I was playing around with this by changing the grid to only show one product per row, and ran into some weird jank when using the default settings; it appears to kick me back to the top when loading more products or something weird like that. Thinking about this, I wonder what our desired experience is, before hydration / when JS is disabled. Would we ideally change the But maybe that also works better with sharing a link: what if I wanted to share with you a product collection on page 3 of pagination? 🤷 |
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 POC @cartogram. I'm positive about it.
I tried to not look at other people's comments to not bias my feedback.
I have some questions:
- How could someone modify the URL to have
page=1
,page=2
instead of the large cursor? I imagine merchants not fond of the large url .e.g fashionova - WIP maybe but I'm noticed some jankiness: https://screenshot.click/20-26-f2gby-8ilm2.mp4
- Is going back and being in the same scroll position part of the implementation? It doesn't seem to be working https://screenshot.click/20-26-f2gby-8ilm2.mp4
getPaginationVariables
forces someone to use the request or url, could there be a reason a dev would want to not use a url for the pagination state?- [nit] suggestion
getPaginationVariables
could have different signature
getPaginationVariables(request, {
first: 4
})
this would allow us to expand variables, or overwriting any output in case it is needed without breaking changes.
6. Maybe this is a limitation from storefront API. Is it possible to implement pagination like this?
7. I wonder if this is relevant. From [google practices on pagination](Give each page a unique URL):
Give each page a unique URL. For example, include a ?page=n query parameter, as URLs in a paginated sequence are treated as separate pages by Google.
I wonder how does this impact 🤔
hasNextPage: boolean; | ||
hasPreviousPage: boolean; | ||
isLoading: boolean; | ||
nextLinkRef: any; |
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.
what is the nextLinkRef
used for?
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 is used to tell the autoLoadOnScroll version what DOM element should cause a load more.
endCursor, | ||
hasNextPage, | ||
startCursor, | ||
hasPreviousPage: undefined, |
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.
On first view, I think I'm lacking understanding of Link.state
. I don't understand on first view what this is doing.
Why is hasPreviousPage: undefined
?
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.
The link state is like a cache. On the first view it is empty.
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 like the simplicity and elegant API area of this approach, especially considering we probably can't use page numbers for pagination.
The implementation itself doesn't seem to work properly yet, though. For example, I only get the "Oxygen snowboard" item when using page size 2, not 4. Plus, scroll seems to be lost when refreshing.
Other things in no particular order:
- It seems to be querying parent layout loaders (root) on scroll, which seems unnecessary. Would it be possible to easily disable this in Remix with this approach?
- I can see two requests for
/products
when scrolling each time, and it seems the result of one of them is not even displayed.
- I assume when refreshing, it should only load items "after" the current cursor? It seems to be loading all of them again 🤔
- It's been mentioned before but I also think it would be good to think how this would play if we want to unload items from the top. Or maybe that would be a different component?
Also, what do you think about separating this in 2 components? One with infinite scrolling and another without it. It might simplify the implementation (or not?) and allow tree-shaking for those who don't need react-intersection-observer
.
if (isLoading) return; | ||
|
||
const nextPageUrl = | ||
location.pathname + `?index&cursor=${endCursor}&direction=next`; |
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.
Curious, what's this index
for?
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.
it's a remix thing for fetcher to refer to index route https://remix.run/docs/en/main/guides/routing#what-is-the-index-query-param
Thanks @lordofthecactus 🙏 Responses to your feedback below:
This is a limitation provided by the Storefront API which uses cursor-based pagination. We could potentially write code to map cursors to pages (which I will explore next), but it would be better to support this at the API layer.
This video and the one below are the same, is this intentional?
It doesn't :( Will address this next.
I'm not sure I understand that use case, but re-routing this question to @benjaminsehl, is this a requirement?
I like it! ❤️
Also forward to @benjaminsehl, but I think no and no we don't want to support that.
It does this (at least currently). |
@frandiox I didn't consider the tree-shaking aspect. @blittle perhaps we do a staggered release, makes even more sense if they are separate components. cc @benjaminsehl for your thoughts too. |
I think there's enough reason for us to not do infinite scroll at first and see how it lands. What do you think? We could also provide an example of how to add infinite scroll manually. Conceptually I'm not fond of providing two separate components. |
b2e1e71
to
c46f927
Compare
c46f927
to
8a72c43
Compare
c46f927
to
f190bb9
Compare
This comment has been minimized.
This comment has been minimized.
|
||
import {Link, LinkProps, useNavigation, useLocation} from '@remix-run/react'; | ||
|
||
type Connection<NodesType> = { |
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 flattenConnection
has a better custom type for this, but it lives in hydrogen react. Hmm, may either be worth copying or export it?
Especially since it can also have the edges
property instead of nodes
and such.
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.
It seems as though I'd need to modify some of the type to get it to properly infer, considering the current Hydrogen React version has an explicit unknown on the nodes/edges types:
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.
More generally, I'm not sure that Pagination will work with edges
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.
It seems as though I'd need to modify some of the type to get it to properly infer, considering the current Hydrogen React version has an explicit unknown on the nodes/edges types:
Those types are just used in the extends
clauses of the function definition; note that there, the keyword infer
is used to infer what the actual type is that is being passed in, which makes it work.
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.
More generally, I'm not sure that Pagination will work with edges
oh really? Why wouldn't it?
|
||
interface PaginationInfo<NodesType> { | ||
/** The paginated array of nodes. You should map over and render this array. */ | ||
nodes: Array<NodesType>; |
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.
Same here; it could be nodes
or edges.nodes
and stuff like that, and we shouldn't prescribe which one is used.
() => | ||
function NextLink(props: Omit<LinkProps, 'to'>) { | ||
return hasNextPage | ||
? createElement(Link, { |
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.
Ah fun. Do we have to use createElement
here or can we just use jsx?
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 was lazy, and haven't yet configured the compilation to handle tsx
() => | ||
function PrevLink(props: Omit<LinkProps, 'to'>) { | ||
return hasPreviousPage | ||
? createElement(Link, { |
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.
same question here
|
||
export function Pagination<NodesType>({ | ||
connection, | ||
children = () => null, |
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.
Hmm, is this the right type for children? I would expect it to return JSX, right?
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.
Honestly perhaps we should warn if there is no child prop
endCursor: Maybe<string> | undefined; | ||
} { | ||
const [nodes, setNodes] = useState(connection.nodes); | ||
const {state, search} = useLocation() as { |
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.
can we do useLocation<{type here}>()
instead?
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.
Sadly, nope, useLocation
doesn't take a generic. I think the cast is being used to coerce state
to be the expected value that we saved. But it's incorrect because that state isn't guaranteed to be there. So it probably should instead have optional properties, and PaginationState
should also have optional properties (forcing null checks):
const {state, search} = useLocation() as {
state?: PaginationState<NodesType>;
search?: string;
};
props: PaginationInfo<NodesType>, | ||
) => JSX.Element | null; | ||
|
||
export function Pagination<NodesType>({ |
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.
Can we add TS docs for this function? It could probably match whatever we have in the docs.
WHY are these changes introduced?
This PR brings a Pagination component into the skeleton template. I am looking for feedback on this approach and for someone to take this on while I am on vacation for the next few weeks.
WHAT is this pull request doing?
<Pagination />
componentusePagination
hook/products
route to the skeleton template that consumes this componentHere is a demo with infinite scrolling
Screen.Recording.2023-04-04.at.18.14.41.mov
Here is a demo with forward and back links
Screen.Recording.2023-04-04.at.18.19.27.mov
Some features this includes:
<Pagination />
is where you can build up your UI such as forward/next buttons and the product/collection card components. The object provided to the children function is of the following:And simple UI might look like this:
HOW to test your changes?
Run the skeleton template and visit
/products
.Next steps