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

Added pagination for nested modules. #2637

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

markfruss
Copy link

Description

Adds pagination to nested modules.

image

Related Issues

None

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2024

CLA assistant check
All committers have signed the CLA.

@ifox
Copy link
Member

ifox commented Jul 25, 2024

Hi @markfruss, pagination is purposefully not on nested listings because it would prevent moving a nested page to a parent page that is on a different pagination page. This change also means that child items are not rendering in the listing anymore, right?

@markfruss
Copy link
Author

Hey @ifox!

I see what you're saying. I think it's an improvement over the default behavior where we can't see all items belonging in a module.

From my testing, the relationships took care of loading child items, the change you mentioned prevents pages loading in children mistakenly as top level items because of the way the paginator loads ranges of elements in the DB.

@ifox
Copy link
Member

ifox commented Jul 25, 2024

Ok makes sense.

I'm not sure I understand your point about "the default behavior where we can't see all items belonging in a module". Can you please clarify? The default behavior has obvious scaling issues since it is not paginated, but it doesn't hide any item, so I'm a bit confused.

@markfruss
Copy link
Author

Without that line, in my testing I would occasionally experience children not honoring their nesting, and ending up on the next page over. Adding the condition to the query builder to pull only top level seemed to mitigate the issue while still correctly pulling in the children.

@ifox
Copy link
Member

ifox commented Jul 25, 2024

Gotcha, by default behavior you mean once you've added the paginator.

A simple solution would be to allow larger perPage values in the case of nested listings so that one can still show the whole tree to be able to move a page from one parent to another.

@ifox
Copy link
Member

ifox commented Jul 25, 2024

You probably know this, but in case you don't or for anyone stumbling upon this thread, the nested modules require a queue to process reordering actions. This is to avoid collisions that would break the tree. The package we use has features to fix the tree programmatically in case it is broken, which can be checked with a helper.

@markfruss
Copy link
Author

Gotcha, by default behavior you mean once you've added the paginator.

A simple solution would be to allow larger perPage values in the case of nested listings so that one can still show the whole tree to be able to move a page from one parent to another.

Are you suggesting to avoid pagination altogether and increase the perPage value on the controller, or to perhaps extend this out to support a larger range / all?
image

@ifox
Copy link
Member

ifox commented Jul 25, 2024

The latter

@markfruss
Copy link
Author

I think that's a good idea.

@markfruss
Copy link
Author

markfruss commented Jul 26, 2024

I'm curious to hear your feedback/thoughts on this.

If 1 is passed back to the paginator, you lose pagination. I'm not sure why it defaults to one, maybe someone could shed some light on it. I think a countable is always passed here though, and it seems to work like it should in my testing.

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.

3 participants