-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add support for Pagy as pagination backend #443
Conversation
Co-authored-by: Ryan Wolfe <ryan.wolfe@strongmind.com>
Co-authored-by: Ryan Wolfe <ryan.wolfe@strongmind.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @tpaulshippy ! 🙇♂️
I have 2 nitpick comments about style, and one around the failing tests in the CI. Let me know if you have any questions!
lib/algoliasearch/pagination/pagy.rb
Outdated
@@ -0,0 +1,21 @@ | |||
unless defined? Pagy | |||
raise(AlgoliaSearch::BadConfiguration, "AlgoliaSearch: Please add 'pagy' to your Gemfile to use pagy_search pagination backend") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise(AlgoliaSearch::BadConfiguration, "AlgoliaSearch: Please add 'pagy' to your Gemfile to use pagy_search pagination backend") | |
raise(AlgoliaSearch::BadConfiguration, "AlgoliaSearch: Please add 'pagy' to your Gemfile to use Pagy pagination backend") |
describe 'Pagy' do | ||
before(:all) do | ||
require 'pagy' | ||
AlgoliaSearch.configuration = { :application_id => ENV['ALGOLIA_APPLICATION_ID'], :api_key => ENV['ALGOLIA_API_KEY'], :pagination_backend => :pagy } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests after this one are failing, because the pagination backend is configured globally by this line, and the return value differs from other pagination backends (others return one value, while pagy returns 2).
I see that Pagy defaults to returning 2 values (Pagy object and the paginated list of items), so we should reset the pagination backend after these tests have run to make the other tests still pass.
lib/algoliasearch/pagination/pagy.rb
Outdated
vars = {} | ||
vars[:count] = total_hits | ||
vars[:page] = options[:page] | ||
vars[:items] = options[:per_page] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vars = {} | |
vars[:count] = total_hits | |
vars[:page] = options[:page] | |
vars[:items] = options[:per_page] | |
vars = { | |
count: total_hits, | |
page: options[:page], | |
items: options[:per_page] | |
} |
- Fix name of pagination backend in error message - Cleanup hash construction to match style guidelines Co-authored-by: Paul Shippy <paul.shippy@strongmind.com>
Hi @DevinCodes, thanks for the feedback, we have addressed your comments in the latest commit! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me, thank you again for the contribution! I'll try to release this early next week 🙂
This is released in the latest version! https://rubygems.org/gems/algoliasearch-rails/versions/2.3.2 Thank you again for the help on this 🙇 |
Describe your change
This change adds support for pagy as a pagination backend.
Co-authored by: Ryan Wolfe
What problem is this fixing?
Currently when using algolia with pagy, we have to use pagy_array which means that we are pulling the entire result set for every page. This is not optimal.
Here is a suggested addition to the README:
Even if we highly recommend to perform all search (and therefore pagination) operations from your frontend using JavaScript,
we support the following as pagination backends:
will_paginate
kaminari
pagy
pagy
To use
:pagy
, specify the:pagination_backend
as follow:Then, as soon as you use the
search
method, the returning results will be a paginated set: