[Feature] Add cursor pagination to Livewire #3215

ariaieboy · 2021-07-01T14:04:24Z

1️⃣ Is this something that is wanted/needed? Did you create an issue / discussion about it first?
#3182
2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
No
3️⃣ Does it include tests, if possible? (Not a deal-breaker, just a nice-to-have)
No, I looked at the default pagination tests they only test that page property is set currectly or Not
since the cursor pagination dont use numbers as refrrence for pagination and use a encoded string based from the data i couldnt add test for it if you guys give me a hint i may can add some test for it.

4️⃣ Please include a thorough description of the improvement and reasons why it's useful.
laravel introduced cursor pagination in laravel v8.41.0 this PR aim to add this feature to livewire pagination system.

with this PR users can replace withPagination trait with withCursorPagination trait and change paginate() method with cursorPaginate() and use cursor pagination instead of regular pagination in livewire

5️⃣ Thanks for contributing! 🙌

nuernbergerA · 2021-07-02T06:00:55Z

Nice PR, the only concern is the version constraint with laravel.

@calebporzio maybe its time to only support the current laravel version (it's not a big deal nowadays to keep your app up to date).
and if you wanna use modern tech (like livewire) update your app, if you cant update you can still use the current (old) livewire version (without updates).

ariaieboy · 2021-07-02T07:44:53Z

Nice PR, the only concern is the version constraint with laravel.

@calebporzio maybe its time to only support the current laravel version (it's not a big deal nowadays to keep your app up to date).
and if you wanna use modern tech (like livewire) update your app, if you cant update you can still use the current (old) livewire version (without updates).

about this PR i don't think the laravel version will be a big concern cause its just a trait that user can add it manually. and we can notice them in docs that this feature only works with laravel 8.41+

But in general I agree with you. And I think not only the Laravel version should be limited to the latest version, but also the php version should be limited to the versions that are actively supported.

danharrin · 2021-07-07T17:23:10Z

Before another pagination trait gets added, is it possible to ensure this one supports multiple pagination instances on a single component from the start? That's LW's biggest failing at the moment in my opinion.

ariaieboy · 2021-07-08T04:52:06Z

Before another pagination trait gets added, is it possible to ensure this one supports multiple pagination instances on a single component from the start? That's LW's biggest failing at the moment in my opinion.

I can add this feature for both pagination traits but I need to change the livewire paginator api and since users maybe publishes the views and uses custom themes it might not be backward compatible so we can't merge it with current version branch.

but still i am looking at this problem and may come with a solution without breaking things

danharrin · 2021-07-08T07:11:25Z

Before another pagination trait gets added, is it possible to ensure this one supports multiple pagination instances on a single component from the start? That's LW's biggest failing at the moment in my opinion.

I can add this feature for both pagination traits but I need to change the livewire paginator api and since users maybe publishes the views and uses custom themes it might not be backward compatible so we can't merge it with current version branch.

but still i am looking at this problem and may come with a solution without breaking things

Yeah, even just adding support on this new trait would be good, since there would be no breaking changes.

ariaieboy · 2021-07-08T08:16:10Z

Before another pagination trait gets added, is it possible to ensure this one supports multiple pagination instances on a single component from the start? That's LW's biggest failing at the moment in my opinion.

I can add this feature for both pagination traits but I need to change the livewire paginator api and since users maybe publishes the views and uses custom themes it might not be backward compatible so we can't merge it with current version branch.
but still i am looking at this problem and may come with a solution without breaking things

Yeah, even just adding support on this new trait would be good, since there would be no breaking changes.

i am working on a new pr that add multiple pagination on single component but @calebporzio need to review and accept it

add method_exists check for $route->getMissing() function

calebporzio · 2021-09-03T13:53:00Z

@ariaieboy - I haven't really dug into this problem, but is it possible to support his in the existing WithPagination trait?

ariaieboy · 2021-09-03T14:30:47Z

@ariaieboy - I haven't really dug into this problem, but is it possible to support his in the existing WithPagination trait?

it can be implemented in the same WithPagination trait but i think keep them seperated is better option.
because cursor pagination is only supported in laravel 8 and by changing the currunt trait we need alot of checks for laravel < 8 and for using cursor pagination developers need to do some tweaks before using it and it have some limitations vs the regular pagination its better to keep them seperate and let the users decide wich kind of pagination they need to add to their components.

Cursor vs. Offset Pagination

ariaieboy · 2021-09-03T14:42:19Z

in this PR #3264 i've added multiple cursor pagination on single component.

i've used an array to define the paginations queryStrings.
if we want use 1 trait for both pagination we can use a key=>value on that array that the key is the queryString and the value is the pagination type and then we can have multiple pagination in one component with both regular and cursor pagination without any breaking changes .

we add page query string as default and every thing will work without any breaking change.

just watch that PR and if that was ok let me know so i work on it to add regular pagination to it and update my PR according to that.

joshhanley · 2021-09-05T21:21:09Z

@ariaieboy I've been working on adding multiple pagination support to the existing with pagination trait that doesn't have breaking changes, but I'm just waiting for my two other pagination PRs to be merged and I will submit it.

calebporzio · 2021-09-07T15:10:14Z

@joshhanley - I think we're good now? I don't see any open PRs for you - yeah? Let me know if you need more from me

calebporzio · 2021-09-09T17:59:43Z

Ok, Josh's other PRs are all merged now. I'm open to supporting cursor pagination - ideally it would be contained within the same trait with minimal forking

…r_pagination

ariaieboy · 2021-09-09T23:50:29Z

@calebporzio
i update my PR and now its works with the same withPagination trait.
i've added 2 tests and in my local i passed all of it please check and confirm the PR.

joshhanley

@ariaieboy just a few comments and changes 🙂 also not sure why the dist/livewire.js is showing up and licence.md. Can you make sure it's up to date with master so they're not showing in this PR?

Thanks!

tests/Browser/Pagination/Test.php

LICENSE.md

src/WithPagination.php

rbase master to the branch

…n use string

ariaieboy · 2021-11-21T22:11:29Z

src/WithPagination.php

@@ -75,8 +86,10 @@ public function resetPage($pageName = 'page')

    public function setPage($page, $pageName = 'page')
    {
-        $page = intval($page);
-        $page = $page <= 0 ? 1 : $page ;
+        if (is_numeric($page)){


since cursor pagination use string as the identifier we should not cast the $page to intger all the time

ariaieboy · 2021-11-21T22:12:16Z

tests/Unit/ComponentPaginationTest.php

@@ -48,20 +48,12 @@ public function double_page_value_should_be_casted_to_int()
            ->call('gotoPage', 2.5)
            ->assertSet('page', 2);
    }
-
-    /** @test */
-    public function string_page_value_should_be_reset()


as i said since cursor pagination use string as identifier we should not reeset strings

ariaieboy · 2021-11-21T22:13:05Z

@joshhanley @calebporzio
ok so i rebase my code to master and fixed all tests
i think we can now merge this to master

calebporzio · 2021-12-08T21:32:01Z

Thanks so much for this @ariaieboy!

ariaieboy added 2 commits July 1, 2021 18:24

wip

be2c6ce

wip

d9dd8f7

ariaieboy mentioned this pull request Jul 8, 2021

add multiple pagination support on single component #3264

Closed

ariaieboy added 3 commits July 23, 2021 11:23

Update Component.php

dde54c4

add method_exists check for $route->getMissing() function

Merge branch 'livewire:master' into master

3229172

Merge branch 'livewire:master' into master

87f9ff5

ariaieboy added 9 commits September 10, 2021 00:38

Merge branch 'livewire:master' into master

e7081ab

Merge branch 'livewire:master' into add_cursor_pagination

9fc8998

added cursor paginator to the withPagination trait

7da7ab6

add tests

fba98b3

add tests

d49fe32

fix tests

4c01902

fix tests for laravel 7

95ef4ed

Merge branch 'master' of github.com:ariaieboy/livewire into add_curso…

cd3db30

…r_pagination

cleanup

4579d8d

joshhanley reviewed Sep 10, 2021

View reviewed changes

tests/Browser/Pagination/Test.php Outdated Show resolved Hide resolved

LICENSE.md Outdated Show resolved Hide resolved

src/WithPagination.php Show resolved Hide resolved

Merge branch 'livewire:master' into master

ac42af6

ariaieboy added 2 commits September 10, 2021 15:22

fix

3d00fcd

Merge branch 'master' into add_cursor_pagination

50a63ff

joshhanley changed the title ~~add cursor pagination to livewire~~ [Feature] Add cursor pagination to Livewire Sep 10, 2021

ariaieboy added 14 commits September 11, 2021 02:47

Merge branch 'master' into add_cursor_pagination

f15aa1d

Merge branch 'livewire:master' into master

bb81f6e

wip

0f5408b

wip

77172ae

added cursor paginator to the withPaginationtrait

1fb3b1a

rbase master to the branch

add tests

d335b15

add tests

7818189

fix tests

3e52f26

fix tests for laravel 7

379d3b0

fix

c8c4ee0

fix conflicts

b2b7596

fix tailwindcss test

2e72d70

fix tests

0868006

remove string_page_value_should_be_reset test because cursorPaginatio…

8e8da4b

…n use string

ariaieboy commented Nov 21, 2021

View reviewed changes

calebporzio merged commit 316783d into livewire:master Dec 8, 2021

ariaieboy deleted the add_cursor_pagination branch December 8, 2021 21:58

nshiro mentioned this pull request Dec 17, 2021

Fix cursor pagination in bootstrap #4406

Merged

Uh oh!

[Feature] Add cursor pagination to Livewire #3215

[Feature] Add cursor pagination to Livewire #3215

Uh oh!

Conversation

ariaieboy commented Jul 1, 2021

Uh oh!

nuernbergerA commented Jul 2, 2021

Uh oh!

ariaieboy commented Jul 2, 2021

Uh oh!

danharrin commented Jul 7, 2021

Uh oh!

ariaieboy commented Jul 8, 2021

Uh oh!

danharrin commented Jul 8, 2021

Uh oh!

ariaieboy commented Jul 8, 2021

Uh oh!

calebporzio commented Sep 3, 2021

Uh oh!

ariaieboy commented Sep 3, 2021

Uh oh!

ariaieboy commented Sep 3, 2021

Uh oh!

joshhanley commented Sep 5, 2021

Uh oh!

calebporzio commented Sep 7, 2021

Uh oh!

calebporzio commented Sep 9, 2021

Uh oh!

ariaieboy commented Sep 9, 2021

Uh oh!

joshhanley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariaieboy Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

ariaieboy Nov 21, 2021

Choose a reason for hiding this comment

Uh oh!

ariaieboy commented Nov 21, 2021

Uh oh!

calebporzio commented Dec 8, 2021

Uh oh!

Uh oh!