-
Notifications
You must be signed in to change notification settings - Fork 3
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 total package count and improved pager #1038
Conversation
5ffcfd6
to
6b91f06
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1038 +/- ##
=======================================
Coverage 55.38% 55.38%
=======================================
Files 103 103
Lines 6021 6021
=======================================
Hits 3335 3335
Misses 2434 2434
Partials 252 252 ☔ View full report in Codecov by Sentry. |
7ea9144
to
a30763e
Compare
1e6bfa8
to
daab09e
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.
Thanks @djjuhasz, and sorry for the late feedback!
You went with a fully fledged pagination, I like the implementation in general but I have three main concerns:
Responsiveness
Mostly covered in my comments below, I'd like to address that in this PR if we go with this full pagination.
Re-usability
We should implement this as a component that could be used in other parts of the dashboard (or easily migrated to other UIs). This can happen in another PR, where we can also think about the following point ...
Performance
While I like to have the option to do full pagination in the API, it was a request in another project, using offset and limit. I think we should try to use cursor based pagination when possible ourselves, specially if you are showing that last page link, that could be heavy on MySQL. With better filtering and sorting options, the next/previous pagination would be better/enough in most cases (like the Github commits list). Or use scroll based pagination in other places (like the Gitlab commits list).
@jraddaoui with regards to performance, I disagree about using cursor based paging instead of offset & limit for the following reasons:
|
@fiver-watson here! |
Running locally, with a clean database, just created 1M packages with the genpkgs script:
|
83c123b
to
2297a7f
Compare
Also tested with 1M packages and an index on the
|
1M packages, offset of
|
1M packages, offset of
|
1M packages, offset of
|
To summarize my tests with offset vs. cursor searches on a set of one million packages sorted by name (descending) and various offsets: Observations:
Trends:
Conclusions:
|
Yeah, that was my understanding too, I am okay removing the last page link. |
@jraddaoui as we agreed in the scrum today, I'll leave the first/last page links on the pager for now. |
Fixes #988: The package list shows the total number of packages found, before paging. Other changes: - Show text indicating which packages are currently displayed as well as the total number of results, e.g. "Showing 1 - 20 of 228" - Show the pager only when there are more results than the page limit, e.g. if 27 results are returned an the page limit is 20 - Include individual page links for up to 7 pages - Show "first" and "last" page links when more than 7 pages of results are returned - Show an ellipsis before or after the page links to indicated there are more pages than the page links show - Hide page links and ellipses on narrow screens
c9d03f2
to
7f26650
Compare
Fixes #988: The package list shows the total number of packages found, before paging.
Other changes: