-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow page navigation after search #146
Conversation
Co-authored-by: Anna Headley <hackartisan@users.noreply.github.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.
We should add a test for the search results pagination bug fixed in this PR.
spec/models/guide_card_spec.rb
Outdated
@@ -53,5 +53,9 @@ | |||
guide_card19 = GuideCard.create(id: 19) | |||
expect(guide_card19.index_page).to eq 2 | |||
end | |||
it 'id 27021 is on page 2702' do | |||
guide_card27021 = GuideCard.create(id: 27021) | |||
expect(guide_card27021.index_page).to eq 2702 |
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.
2703 seems right. we can update this test or remove it.
Note that these cards must be ordered by id, not heading. This is counterintuitive, because it results in a thing that might not look like alphabetical order. However, it is the order of the card catalog, which is wanted by the stakeholder. Co-authored-by: Bess Sadler <bess@users.noreply.github.com>
@@ -3,12 +3,17 @@ | |||
# controller for GuideCards | |||
class GuideCardsController < ApplicationController | |||
def index | |||
@guide_cards = GuideCard.page(params[:page]) | |||
@guide_cards = GuideCard.order(:id).page(params[: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.
There's a great commit message about why we're ordering by id. It might be nice to add a similar comment into the code.
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.
I have added this logic to our controller as a comment. Thank you!
Co-authored-by: Anna Headley <hackartisan@users.noreply.github.com> Co-authored-by: Bess Sadler <bess@users.noreply.github.com>
Ref #22