-
Notifications
You must be signed in to change notification settings - Fork 2
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
Revise pagination item calculation #1829
Conversation
27ecc51
to
b802d7a
Compare
a54c2b4
to
6829c62
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1829 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 70 70
Lines 1272 1303 +31
Branches 477 482 +5
=========================================
+ Hits 1272 1303 +31 ☔ View full report in Codecov by Sentry. |
}); | ||
}); | ||
|
||
function* configurations() { |
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 test above checks a few specific examples against expected results. The test below is a property test that generates a range of configurations and for each verifies that certain properties are satisfied for every possible current
page.
75b35b4
to
91c7ec9
Compare
|
||
function isStrictlyIncreasing(list) { | ||
return list.every((item, idx) => idx === 0 || item > list[idx - 1]); | ||
} |
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.
"strictly increasing" as in "a strictly increasing function"
91c7ec9
to
4dd8964
Compare
Revise the calculation of the set of page numbers so that the number of pagination items (page numbers and elided-page markers) is consistent regardless of the current page. If each item is rendered at a consistent width, this keeps the controls in the same place as the user navigates through the page list, avoiding mis-clicks due to pages jumping around.
4dd8964
to
f96444c
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.
The algorithm is maybe not super easy to understand at a glance, but it does bring the benefits you describe here, and the new tests help a lot documenting it.
// Pages elided after current. | ||
{ | ||
current: 1, | ||
total: 4, | ||
expected: [1, 2, null, 4], | ||
}, | ||
|
||
// Pages elided before current. | ||
{ | ||
current: 4, | ||
total: 6, | ||
expected: [1, null, 3, 4, 5, 6], | ||
}, |
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 test is great, because it helps document the functionality, which is pretty complex.
However, it made me realize that, considering the elision in a 6-page pagination ([1, null, 3, 4, 5, 6]
), I would have expected no elided pages for a 4-page pagination ([1, 2, 3, 4]
) regardless of current page, but it seems we do elide ([1, 2, null, 4]
).
Can you explain the algorithm to me as if I were a five-year-old 😅?
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.
In prose:
- If there is one page, return an empty list
- Calculate if there are elided pages before the current page
- If no, create a list of all pages before the current page. If yes, create a list of the pages before the elided range, then
null
, then pages after the elided range. - Add the current page to the list
- Calculate if there are elided pages after the current page
- If no, append all pages after the current page. If yes, append a list of pages after the current page, before the elided range, then
null
, then pages after the elided range - Calculate the maximum number of items we might show, for all possible values of the
current
page - While the current number of items in the list is less than the maximum, expand the list by inserting an extra page number and "shrinking" the elided range. We preferentially take the page numbers from the elided range after the current page, but if that is empty, take them from the elided numbers before the current range.
Revise how the pagination items (page numbers and elided page markers) are calculated, such that the number of items rather than the number of numbered pages remains consistent as the user navigates through the list. Provided each item is rendered with a roughly consistent width, this keeps the overall width of the control and the position of child components consistent when navigating through the list.
When this middle controls have a consistent width, the Pagination also works better in cases where the Prev/Next controls are adjacent to the middle items, rather than aligned with the edge of the content area above (the table here).
Testing:
Go to http://localhost:4001/navigation-pagination, navigate through the list. Observe that the number of items displayed is the same regardless of current page. Compare with https://patterns.hypothes.is/navigation-pagination where this is not the case.