Skip to content

Filter article exports by issue and implement pagination#127

Open
ajrbyers wants to merge 1 commit into
mainfrom
b-export-filter-by-issue
Open

Filter article exports by issue and implement pagination#127
ajrbyers wants to merge 1 commit into
mainfrom
b-export-filter-by-issue

Conversation

@ajrbyers

@ajrbyers ajrbyers commented Jun 10, 2026

Copy link
Copy Markdown
Member

This PR:

  1. Adds queryset based pagination to avoid loading 1000s of records onto the page, removes datatables.
  2. Adds a filter by issue option

@StephDriver StephDriver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Andy, I'm a little confused by this PR.

  1. pagination was better before:

OLD:

Image

New:

Image
  1. I like the change of records per page nearer pagination. But previously the records per page options had 5,10,50,25 (yes, not in order!) but now it's 10,25,50,100. Why the loss of 5?

  2. The default sort has been lost. Previously this defaulted to sorting by ID. Now there is no default sort. Intentional?

  3. The export is in a completely different order of fields within the csv. I did a cell by cell comparison to check all are the same (and they are) but this threw me and I think could throw users if they are used to plugging the export into other processes- is there a reason for reordering it?

  4. BUT, the filter by issue is NICE! Especially as collections also show up there.

So I cannot review this as I feel unsure what it was intended to do. There is no linked issue, and no info in the PR conversation.

@StephDriver StephDriver assigned ajrbyers and unassigned StephDriver Jun 11, 2026
@ajrbyers

ajrbyers commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

More info:

  • The sort order will fallback to the default for the model, which is fine
  • I don't think 5 records is particularly useful
  • The export order has nothing to do with this PR
  • The original pagination isn't actual pagination and is just databases, so you're loading hundreds if not thousands of record onto the page, slowing it down massively. Feel free to compare load times.

@ajrbyers ajrbyers assigned StephDriver and unassigned ajrbyers Jun 11, 2026
@StephDriver

StephDriver commented Jun 11, 2026

Copy link
Copy Markdown
Member

@ajrbyers thanks.

TLDR: 1 change requested - put the "showing X of Y entries" back into the pagination row. details below.

More info:

  • The sort order will fallback to the default for the model, which is fine

👍

  • I don't think 5 records is particularly useful

Ok, that's fine - sounds like it was deliberate, and I certainly approve of having the values in numerical order now.

  • The export order has nothing to do with this PR

ok. was comparing to main. And sort order isn't the same.

  • The original pagination isn't actual pagination and is just databases, so you're loading hundreds if not thousands of record onto the page, slowing it down massively. Feel free to compare load times.

OK - can the "showing X of Y entries" part that was shown previously be put back? I think that's useful.

@StephDriver StephDriver assigned ajrbyers and unassigned StephDriver Jun 11, 2026
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