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

Replace kaminari with pagy #681

Merged
merged 5 commits into from
Oct 7, 2023

Conversation

amadayami
Copy link
Contributor

Context

Checklist

  • Tested Mobile Responsiveness
  • Added Unit Tests
  • CI Passes
  • Deploys to Heroku on test Correctly (Maintainers will handle)
  • Added Documentation (Service and Code when required)

Screenshots

Before

Screen Shot 2023-05-23 at 20 21 01

After

Screen Shot 2023-05-23 at 20 19 58

amadayami and others added 2 commits May 23, 2023 20:00
Copy link

@Joranhezon Joranhezon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for working on this. (:

I have some suggestions, but nothing major. ✌️

Comment on lines 13 to 16
use :pagy,
items_param: :per_page,
items: 10,
max_items: 100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a comparison here we could make to keep the = instead of !=?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! It can be rewritten in this way to maintain the = that was there previously. This is functionally the same as using !=.

Suggested change
!= 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".

@amadayami
Copy link
Contributor Author

The check failed after the CI errored in the build phase. Would someone be able to rerun the check? Thanks 😄

@DeeDeeG
Copy link
Contributor

DeeDeeG commented Oct 5, 2023

Hi @amadayami,

I resolved the main issue (where tests were not getting to run at all) on our main develop branch. With that change on this branch, I assume all but one test would pass, since it seems to rely on the Google Maps API working, and there's a bit of a quota or billing issue with that at the moment that's affecting the main site as well.

(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 develop branch to this one, that should get CI mostly working again. (I can rebase or merge it for you, if you rather a maintainer take care of it.) And if it's the same one failure as we're seeing on develop branch right now, then I'd know it isn't this particular PR's fault.

@amadayami
Copy link
Contributor Author

Hi @DeeDeeG
I have merged my branch with the updated develop branch and had no merge conflicts. I addressed the rubocop issues, but the one error you mentioned was still popping up (./spec/features/restrooms_spec.rb:45).

Copy link
Contributor

@DeeDeeG DeeDeeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

@DeeDeeG DeeDeeG merged commit e928fc2 into RefugeRestrooms:develop Oct 7, 2023
1 check failed
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.

Replace kaminari with pagy
3 participants