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

Remove classic pagination #5205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AntonKhorev
Copy link
Collaborator

@AntonKhorev AntonKhorev commented Sep 12, 2024

This removes classic pagination library that's only used for changeset elements pagination:

  • necessary parts of its Paginator class moved to controller's ElementsPaginator
  • necessary parts of PaginationHelper module moved to sidebar_classic_pagination helper

@tomhughes
Copy link
Member

Shouldn't we just be converting changesets to use the same pagination logic as everything else, rather than copying parts of classic pagination into the controller just so we can pretend we got rid of it?

@AntonKhorev
Copy link
Collaborator Author

AntonKhorev commented Sep 18, 2024

Shouldn't we just be converting changesets to use the same pagination logic as everything else

That depends on how significant are the differences between changeset elements and everything else. The differences are:

  • Changesets have an upper bound on the number of elements. There can be a really large and growing number of traces etc. If you wanted to use numbered pages and go to page N, that N could be huge and you'll have to skip a huge number of record with offset. But for changeset elements N can't get higher than 10000 / page size.
  • Changesets don't change once closed. For a list of traces it doesn't make mach sense to be on page 1, 2, 5 etc because new traces are added and old ones are pushed off the page. Changeset elements are not going to be pushed of their page.
  • Traces etc have a well-defined order represented by a single id, changeset elements don't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants