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

Pagination - Fix missing @currentPageSize argument in the "Compact" variant with SizeSelector #1724

Merged
merged 8 commits into from Oct 19, 2023

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Oct 16, 2023

📌 Summary

While working on #1700 I completely forgot that the status of the SizeSelector for the Pagination::Compact needs to be handled too, depending if the component is "controlled" or not (same as for the Numbered variant).

Context: https://github.com/hashicorp/design-system/pull/1700/files#r1357503235

🛠️ Detailed description

In this PR I have:

  • updated the logic for “Compact” variant of Pagination to expose @currentPageSize and handle controlled/uncontrolled changes
  • done some small tweaks
    • converted showLabels getter to simple tracked variable (for consistency with the “Numbered” implementation
    • removed useless/leftover variant assignment in “Numbered” variant of Pagination
    • renamed hasRouting to isControlled for consistency with the Tabs component (HDS-2648)
  • added to the Pagination showcase relevant use case and demo for the Compact variant with SizeSelector
  • updated integration + acceptance tests
  • updated “Component API” and "How to use" documentation for Pagination

👉 👉 👉 Previews:

🔗 External links

Jira tickets:


👀 Component checklist

  • Percy was checked for any visual regression
  • A11y tests have been run locally (yarn test:a11y --filter="COMPONENT-NAME")
  • A changelog entry was added via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Oct 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Oct 19, 2023 2:10pm
hds-website ✅ Ready (Inspect) Visit Preview Oct 19, 2023 2:10pm

@hashibot-hds hashibot-hds added packages/components docs-website Content updates to the documentation website labels Oct 16, 2023
@didoo didoo force-pushed the pagination-compact-expose-currentpagesize branch from 853dc22 to fbe43b3 Compare October 16, 2023 12:02
@didoo didoo force-pushed the pagination-compact-expose-currentpagesize branch 3 times, most recently from e85222c to a398995 Compare October 18, 2023 09:37
@didoo didoo force-pushed the pagination-compact-expose-currentpagesize branch from a398995 to e41e778 Compare October 18, 2023 09:37
@didoo didoo force-pushed the pagination-compact-expose-currentpagesize branch from e41e778 to 65ebe6d Compare October 18, 2023 10:06
@didoo
Copy link
Contributor Author

didoo commented Oct 18, 2023

While working on this ticket, I had a meeting with @andrew-hashicorp to discuss some possible issues with using a "page size" in the context of a cursor-based pagination.

Here are the findings, for future reference.

The problem

Using the "page size" in a cursor-based pagination can lead to increased complexity and, more importantly, potential UX/usability issues. For example when the cursor is prev and the page size is changed. Let's take the example of the showcase, where we have 39 records sorted by ID, where ID goes from 1 to 39. Let's imagine we are at the first render, so the cursor is next, the first record is 1 and the size is 5 so the user sees the records 1-5. Now the user clicks next, the index is moved to 6 and the user sees the records 6-10. Now the user clicks prev, the cursor is now prev, the cursor is prev but the index is still 6, and the user sees the records 1-5. Now what happens if the user changes page size, from 5 to 10 and then from 10 to 30? The (code) logic says to take the previous 10 (or 30) records before the record 6, but there are only 5 records before, so the user still sees only 5 records (1-5) even if they change the page size from 5 to 10 and from 10 to 30. The functionality seems broken from the user perspective, because the behaviour is different from what they expected.
In a similar way, if the user is at cursor prev index 26 with page size 5 they will see records 21-25 but if they change page size from 5 to 10 they will see records 16-25, not 21-30 as they would probably expect.

The solution

We could not find an "easy" and obvious solution.

@andrew-hashicorp has implemented some workaround but "the tradeoff is that it requires an additional nextPage or prevPage API request, so at worst you need to fetch 2x max pageSize."

This is part of the nature of how cursor-based pagination works, and it's hard to reconcile what a user would expect with how the intrinsic logic works. I imagine there may be implementations out in the wild the we could use as reference (eg this document is very detailed: https://jsonapi.org/profiles/ethanresnick/cursor-pagination/ but doesn't explain how this issue is handled, also this done explains in detail how cursor-pagination works: https://engage.so/blog/a-deep-dive-into-offset-and-cursor-based-pagination-in-mongodb/).

Conclusion

With @andrew-hashicorp we have agreed that the implementation in this PR (in which on page change we pass null as cursor direction, so that consumers know that they need to handle it as special case) makes sense, and can be moved forward to review. I have also added a banner warning the consumer of this issue, in the documentation, to make the problem explicit/visible.

But. I think one of the problems is that the logic of handling the cursor-based pagination is handled (partially) client-side, in our products, while many of the examples I've found online delegate the entire logic to the server (they just act as mediator, sending the cursor/index/pagesize parameters to the API and letting the backend handle the logic for them; the backend has direct access to the data, so it can better handle these situations without extra API calls; when the client receives the data, it just do basic manipulation and render it, without changing the list itself).

In any case, I'll discuss this issue with the rest of the HDS engineering team, and we'll come up with a decision if move forward this PR or not.

@jgwhite
Copy link
Contributor

jgwhite commented Oct 18, 2023

@didoo and @andrew-hashicorp I think the solution you’ve come up with here is totally appropriate.

In my opinion, cursor-based pagination has benefits (and in some cases is the appropriate choice) but is intrinsically unintuitive. I would encourage any team facing this dilemma to ask themselves “can we instead invest our energy in implementing search/filtering that is so good that users only need use the pagination controls as a last resort?”.

Copy link

@jesdavpet jesdavpet left a comment

Choose a reason for hiding this comment

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

Great write up @didoo ! 😃 🙌

💯 Totally agree with @jgwhite's comments WRT the inherent awkwardness of cursor based pagination, and a strong preference for search/filtration UX and API capabilities.

End of the day, nobody really wants to page through a large set of results, and IMO we're going to get diminishing returns on our efforts to make such a pagination UX ideal.

Obviating the need to solve problem(s) with that interaction by employing search/filtration seems like a good thing for application teams to consider by default, only resorting to cursor pagination for edge cases.

@didoo
Copy link
Contributor Author

didoo commented Oct 18, 2023

@heatherlarsen see Jamie/Jesse comments above. Do you think we could add this as "guidance" in the Pagination documentation? If you agree, I can create a Jira ticket for that.

@WenInCode
Copy link
Contributor

I agree with this approach. In HCP Consul we are clearing page tokens on filter changes but not currently on page size changes. It works in that nothing errors, but as you mentioned above, it doesn't always end up with the expected UX. I am in favour of your suggestion, as it provides a more reliable/expected UX with regards to cursor pagination.

@andrew-hashicorp
Copy link

andrew-hashicorp commented Oct 18, 2023

But. I think one of the problems is that the logic of handling the cursor-based pagination is handled (partially) client-side, in our products, while many of the examples I've found online delegate the entire logic to the server (they just act as mediator, sending the cursor/index/pagesize parameters to the API and letting the backend handle the logic for them; the backend has direct access to the data, so it can better handle these situations without extra API calls; when the client receives the data, it just do basic manipulation and render it, without changing the list itself).

Just to add further detail to @didoo's excellent summary, pretty much everything works as expected except for the case of staying at the "top" of the page when changing the page size after going to a previous page (which, as a user, is what I would expect). This is because on a previous page, you don't have access to a token that will display the first item if a different page size is selected and is just a result of how cursor-based pagination works.

For example:

I have a list of 20 items, numbered 1-20.

  • First page I see: 1 - 5
  • Click next page, I see: 6 - 10
  • Click next page, I see: 11 - 15
  • Click previous page, I see: 6 - 10 - At this point, I would like to change pageSize = 10, and I would expect to see items 6-15.

This is where the limitation of cursor-based pagination appears - I have no token that references 6. To display 6-15, I would need page_size=10 and either next_page_token=5 or prev_page_token=16.

What I do have access to:

  1. in the URL, the following query params: ?page_size=5&prev_page_token=11
  2. I can calculate the previous and next links:
    prev: ?page_size=5&prev_page_token=6
    next: ?page_size=5&next_page_token=10

So the only way to obtain a token that references 6 is to make a prev page request of size 1 where 5 will be the response's nextPageToken. Example.

Each change in pageSize will require an additional API request of 1 result.

How the server can help here is by providing certain additional metadata (i.e. how JSON API does it) - this will provide a token that contains the exact reference to each item in question and will provide the client the complete information to form a proper request without needing to fire off an additional request to see the next or previous page.

Alternately a non-JSON API spec method would be to provide an API endpoint that returns the next/prev tokens without the actual data in order to facilitate the workaround without sending the unnecessary 1 result.

@didoo didoo merged commit 52c1a62 into main Oct 19, 2023
15 checks passed
@didoo didoo deleted the pagination-compact-expose-currentpagesize branch October 19, 2023 14:49
@hashibot-hds hashibot-hds mentioned this pull request Oct 19, 2023
@heatherlarsen
Copy link
Contributor

@didoo Someone on the team will need to spend a bit more time thinking through this from a usability perspective, but a ticket to track that work would be great. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants