Support multiple Pagination components on the same page #2649
Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
| // Handle non-namespaced params (empty string namespace) | ||
| if (activeNamespaces.includes('')) { | ||
| params.delete('cursor'); | ||
| params.delete('direction'); | ||
| } | ||
|
|
||
| activeNamespaces | ||
| .filter((namespace) => namespace !== '') | ||
| .forEach((namespace) => { | ||
| const cursorParam = `${namespace}_cursor`; | ||
| const directionParam = `${namespace}_direction`; | ||
|
|
||
| params.delete(cursorParam); | ||
| params.delete(directionParam); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Optionally, you could roll this into one by doing:
const namespacePrefix = namespace === '' ? '' : `${namespace}_`
const cursorParam = `${namespacePrefix}cursor`;
const directionParam = `${namespacePrefix}direction`;
Though tbh it would have been fine to give no special treatment to the empty namespace, and just used _cursor and _direction when working with it (although granted that might look ugly in the URL)
There was a problem hiding this comment.
Yess much cleaner, thanks!
There was a problem hiding this comment.
And yeah I thought _cursor was a bit of an eye sore + would break existing links when folks upgrade.
There was a problem hiding this comment.
I tried it out using the tophat examples, and it doesn't seem to behave correctly. Also I'm getting a warning/error in the console about shared namespaces. But the namespace is different passed to each.
See the video: https://screenshot.click/02-49-qtwoz-9ozrw.mp4
|
Found a bug with the load previous button. For some reason, click on one of the previous load button will make the other list to load all the previous pages. Expected: Clicking a load previous button should only affect its own list Screen.Recording.2024-12-05.at.1.06.26.PM.mov |
| navigate(window.location.toString(), { | ||
| replace: true, | ||
| state: {nodes: undefined, pageInfo: undefined}, | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
This completely breaks. It ends up triggering a duplicate URL nav:
There was a problem hiding this comment.
Additionally, I don't think it will work properly if we fix the URL navigation, because it will erase the navigation state even for soft transitions. This means the paginated state will disappear when going back/forth, breaking scroll position.
In my opinion the issue mentioned by Helen is not that big of a deal. It only happens if you hard refresh on a page, then have click next or previous on one of the elements. And even then, it's not bad data getting loaded, just data from both paginated components loading at the same time. Jarring yes, but not rendering bad data at all.
There was a problem hiding this comment.
wow thank you for catching this!
|
@scottdixon - @blittle and I chatted and agree that we're fine with the load previous bug for now. There doesn't seem to be an easy way to revert window history state to that exact state we want. I have committed an revert on that |
WHY are these changes introduced?
Enables the use of multiple
<Pagination />components on a single page.Resolves https://github.com/Shopify/hydrogen-internal/issues/124
WHAT is this pull request doing?
namespaceprop on the<Pagination />component. If provided, the namespace value prepends the URL parameters - as in<namespace>_cursorand<namespace>_direction.cursoranddirectionparams are used.namespaces are stored in the location state so we can:getPaginationVariablesutility to accept an optional namespace.HOW to test your changes?
app/routes/collections.all.tsxwith this contentapp/components/PaginatedResourceSection.tsxwith this contentnamespace.Checklist