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

Adds support for pagy #23

Merged
merged 8 commits into from
Apr 30, 2019
Merged

Adds support for pagy #23

merged 8 commits into from
Apr 30, 2019

Conversation

kurenn
Copy link
Member

@kurenn kurenn commented Jun 5, 2018

What does this PR do?

  • Adds pagy support
  • Fixes a duplicate variable name for configuration
  • Improves the loading for the pager configuration for each pager

@kurenn kurenn mentioned this pull request Jun 5, 2018
@ddnexus
Copy link

ddnexus commented Jun 5, 2018

@kurenn a couple of things:

  • I see that the :page param is hard-coded in the params merge. Is that the standard way for headers and links, regardless the fact that the page might be passed around as a different param name, or the param :page is the only one supported?
  • The PR doesn't seem to update the README

@kurenn
Copy link
Member Author

kurenn commented Jun 6, 2018

@ddnexus

  • You are right about the hard-coded params, maybe we can add a page_param_name as a configuration option, in case someone decides to customize it, maybe on another PR?
  • Sure, I will update that, I was just to excited to see it working I just push the code

@ddnexus
Copy link

ddnexus commented Jun 6, 2018

Yup! I received explicit requests for that feature in Pagy, and it is available for WillPaginate and Kaminari too. Here is the code that handle that in Pagy:

def pagy_url_for(page, pagy)
  p_vars = pagy.vars; params = request.GET.merge(p_vars[:page_param] => page, **p_vars[:params])
  "#{request.path}?#{Rack::Utils.build_nested_query(pagy_get_params(params))}#{p_vars[:anchor]}"
end

@ddnexus
Copy link

ddnexus commented Jun 7, 2018

@cseelus can add more details about the use cases...

@cseelus
Copy link

cseelus commented Jun 7, 2018

From a (resolved) Pagy issue:

The ability to add arbitrary params similar to will_paginates :params option or Kaminaris option with the same name would be helpful for a number of use cases:

  • […] (not applicable for API pagination)
  • Passing search/form params around, when Pagy is used in combination with a search function
  • Passing in a controller/action name (see will_paginate or Kaminaris docs)

@kurenn kurenn changed the title Support/pagy [WIP] Adds support for pagy Jun 8, 2018
@kurenn
Copy link
Member Author

kurenn commented Jun 8, 2018

Thanks for all the feedback @ddnexus and @cseelus, I'll keep working on this but just focusing on the pagy support as it is, and then on another PR I can work on the dynamic param method, how does that sound.

I will create an issue for that

@kurenn kurenn mentioned this pull request Jun 8, 2018
@cseelus
Copy link

cseelus commented Jun 10, 2018

@kurenn Pagy support does work nicely btw, tested it locally with a production Rails app + Postman as well as an iOS app that consumes the apps API.

Thanks so far!

@cavi21
Copy link

cavi21 commented Sep 21, 2018

hi everyone! wondering if there is a plan to merge this into master before the #24 is closed? because as @cseelus said Pagy support does work nicely btw so maybe this could be merge.

Also if I can help with it or to look into something, count with me :)
Thanks! awesome work

@kurenn kurenn changed the title [WIP] Adds support for pagy Adds support for pagy Sep 26, 2018
@kurenn
Copy link
Member Author

kurenn commented Sep 26, 2018

I think this PR is ready to merge, I updated the README, I'm creating a new issue regarding the :page param name to be dynamic as suggested by @ddnexus

@csaurav
Copy link

csaurav commented Feb 2, 2019

Any update on this ? Will this be merged ?

@kurenn kurenn merged commit f727047 into master Apr 30, 2019
@kurenn kurenn deleted the support/pagy branch April 30, 2019 23:03
@kurenn
Copy link
Member Author

kurenn commented Apr 30, 2019

It is done @csaurav ! 🎉 sorry for the delay! the version is 0.3.0

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.

None yet

5 participants