-
Notifications
You must be signed in to change notification settings - Fork 27
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
Pagination functionality. #26
base: master
Are you sure you want to change the base?
Conversation
You need to limit the database query too. So your public function index(Request $request) {
$this->repo->paginate($request->limit);
} You approach also restricts a users ability to use |
Humm...
This pagination implementation needs to know how many records are. There is a simplePagination that does not require to know that. Works well on Lumen, where defaults pagination was not giving right links. |
We can work this out for lumen for sure, it just needs to be cleaned up and go the correct place. We might make a class for lumen or a trait for lumen methods. |
Laravels magic resolves the page parameter from the $request, same we doing with limit. make a test, for example with simple url , check what happens with rest of parameters when you do the paginate using laravel defaults. Also, have a look at this:: https://github.com/rlacerda83/lumen-api-query-parser/wiki/Usage with a url like this:
Laravel returns:
but I think this is more convenient:
|
i would suggest to not "bind" this to specific request parameter (e.g., I think, these query parameters should be customizable in the |
Check this out. (Haven't tested, maybe needs a fix)
My recommendations its to remove all $perPage, references. clean all that and just just the new function to read directly from request this 2 vars: limit + page.
What I have seen in other libraries its people gives options to set this vars names, I don't think its necessary, you can always overwrite them.
Its up to you, my suggestions:
{@inheritdoc}
Simple the better.