Added pagination for nested modules.

area17 / twill

Twill is an open source CMS toolkit for Laravel that helps developers rapidly create a custom admin console that is intuitive, powerful and flexible. Chat with us on Discord at https://discord.gg/cnWk7EFv8R.

https://twillcms.com

Apache License 2.0

3.77k stars 576 forks source link

Added pagination for nested modules. #2637

Open markfruss opened 3 months ago

markfruss commented 3 months ago

Description

Adds pagination to nested modules.

Related Issues

None

CLAassistant commented 3 months ago

All committers have signed the CLA.

ifox commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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?

ifox commented 3 months ago

The latter

markfruss commented 3 months ago

I think that's a good idea.

markfruss commented 3 months ago

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

It looks like -1 passed to the backend bypasses the paginator.
I've added a new prop isShowEverythingEnabled which defaults to false on the paginator vue component, which should keep it fully backwards compatible.
I only added an English entry for the translation.
Probably the change that would need the most examination is the backend handling of this line: https://github.com/area17/twill/pull/2637/files#diff-8a3084d617335690e3d80eadc43ada1afa56002be98b5b69ca778ead9a4849ecR1772

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.