Replace kaminari with pagy #681
Conversation
Add newline character for rubocop
app/controllers/api/v1/restrooms.rb
Outdated
| use :pagy, | ||
| items_param: :per_page, | ||
| items: 10, | ||
| max_items: 100 |
There was a problem hiding this comment.
I noticed these lines are being reused in each desc, would it be possible to set this param for all of them outside of each example? Kinda like line 5 on the old file.
Something like use :page, items_param :per_page, items: 10, max_items: 100 on line 5 ofthis file.
There was a problem hiding this comment.
Yes, I have changed it so that those parameters are written as a class constant for the descs to reference instead and have committed that change. Grape Pagy does not appear to have anything equivalent to the old line 5, so this would be the best way to extract those parameters.
| @@ -26,20 +26,20 @@ | |||
| .col-sm-12.headroom | |||
| %ul{:class => "restrooms-list", :id => "list"} | |||
| = render @restrooms | |||
| = paginate @restrooms | |||
| != pagy_nav @pagy | |||
There was a problem hiding this comment.
Is there a comparison here we could make to keep the = instead of !=?
There was a problem hiding this comment.
Yes! It can be rewritten in this way to maintain the = that was there previously. This is functionally the same as using !=.
| != pagy_nav @pagy | |
| = pagy_nav(@pagy).html_safe |
Originally, the != evaluates the Ruby without ever sanitizing the HTML. Rewritten, the = is maintained but it's assumed to be safe by marking it with html_safe. It has to be done in this format because pagy is an "agnostic pagination gem it does not use the specific html_safe rails helper on its output".
|
The check failed after the CI errored in the build phase. Would someone be able to rerun the check? Thanks 😄 |
|
Hi @amadayami, I resolved the main issue (where tests were not getting to run at all) on our main (Other maintainers have access to the Google API dashboards, and I don't, so I can't do anything about that personally, though I have pinged the other maintainers.) Anyhow, if you'd like to rebase on, or merge |
|
Hi @DeeDeeG |
There was a problem hiding this comment.
Thank you @amadayami for the code contribution and @Joranhezon for the help with review!
I tested it locally and it appears to work as intended. Tests pass other than the one with the known issue at the moment unrelated to this PR.
Looks good to me. Merging! (Thanks again!)
Context
Checklist
Screenshots
Before
After