Add Lucky::Paginator #1020
Add Lucky::Paginator #1020
Conversation
|
This will be awesome! |
|
Cursor based pagination is becoming more popular. What do you think about adding cursor based pagination support or an interface to implement it yourself? |
|
@wontruefree Definitely in the pipeline! I prefer cursor based for a lot of stuff. Doesn't work for everything, but when it does it is awesome! |
|
Agreed it does not work for everything. Also can add tons of complexity in edge cases. that is the reason I mentioned just an interface for it. |
paulcsmith
force-pushed
the
pcs/paginator
branch
from
e22984c
to
3a6e184
Compare
paulcsmith
force-pushed
the
pcs/paginator
branch
from
6246c0e
to
e1cdf46
Compare
paulcsmith
force-pushed
the
pcs/paginator
branch
from
e1cdf46
to
0f92e66
Compare
There was a problem hiding this comment.
It's a lot going on, but it all looks good from what I can tell. This is pretty huge! I love that you included the Bootstrap and Bulma as built-in components. That will help people to extend as needed.
I left some comments on the specs, but otherwise I think we can get this in! Good job 🎉
| pages, records = Paginatable.new(page: "2").call | ||
|
|
||
| pages.page.should eq(2) | ||
| pages.per_page.should eq(25) |
There was a problem hiding this comment.
Just as a safety, should we still have pages.total.should eq(2)?
| pages, records = PaginatableWithOverriddenMethods.new.call | ||
|
|
||
| pages.page.should eq(2) | ||
| pages.per_page.should eq(10) |
There was a problem hiding this comment.
Same. Maybe having pages.total.should eq(5)?
| page: paginator_page, | ||
| per_page: per_page, | ||
| item_count: query.clone.reset_order.reset_limit.reset_offset.select_count, | ||
| full_path: context.request.resource |
There was a problem hiding this comment.
So with this, if you need to override what your page link paths are, you'd just pass in the string? full_path: "/whatever"
There was a problem hiding this comment.
In this case you can't since the method is hardcoded to using the current path. We could add a setter for full_path or an option on the paginate method, but I think this is a good start for MVP since I think 99% of the time you want to paginate the same url
| begin beginning : Int32 = 0, | ||
| left_of_current : Int32 = 0, | ||
| right_of_current : Int32 = 0, | ||
| end ending : Int32 = 0 |
There was a problem hiding this comment.
Hah yeah me too. Glad it did though!
| begin beginning : Int32 = 0, | ||
| left_of_current : Int32 = 0, | ||
| right_of_current : Int32 = 0, | ||
| end ending : Int32 = 0 |
There was a problem hiding this comment.
Hah yeah me too. Glad it did though!
| page: paginator_page, | ||
| per_page: per_page, | ||
| item_count: query.clone.reset_order.reset_limit.reset_offset.select_count, | ||
| full_path: context.request.resource |
There was a problem hiding this comment.
In this case you can't since the method is hardcoded to using the current path. We could add a setter for full_path or an option on the paginate method, but I think this is a good start for MVP since I think 99% of the time you want to paginate the same url
|
Added the extra checks in f4df5a1 |
This will make it easy to paginate with Lucky. We (and others) can extend this with new goodies since the paginator is decoupled from Avram. This works very very similarly to the Ruby Pagy gem: https://github.com/ddnexus/pagy but with some modifications to link generation and the provided methods.
Still to do:
seriesto help generate gaps and page windowsProbably later on: