-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix result limit and sort templates. #3031
Conversation
Mink tests are passing for me. |
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 agree that changing the limit shouldn't reset the sort -- that seems like a possible copy and paste bug. But looking into this raised another issue: should changing both sort and limit reset the page number? Right now, if you use these controls on page 2, you remain on page 2, even though the meaning of "page 2" has been totally redefined by the changed parameter.
Maybe that's an issue better solved separately -- I think the fixes here make things better even without fixing that problem -- but I thought I'd leave this open until we could discuss. Let me know if you think it's worth tackling that here or starting a separate PR.
I should also note that this PR is going to conflict with #2814 when we get that far, but I agree that solving this for the current dev branch in the meantime still makes sense. We should just make sure that any changed behavior/fixes get correctly merged into that other PR after this work is completed. |
@demiankatz I decided to add filtering of the page parameter in both templates, now here to avoid having to revisit this later. :) |
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.
As discussed in Slack, this looks good to me, but I'll hold off on merging until you've had time to add tests.
Tests now added. |
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.
Thanks, @EreMaijala -- one small suggestion to hopefully make this more future-proof. I was hoping to make the change myself and test it today, but I never quite got there before running out of time!
module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchLimitTest.php
Outdated
Show resolved
Hide resolved
module/VuFind/tests/integration-tests/src/VuFindTest/Mink/SearchSortTest.php
Outdated
Show resolved
Hide resolved
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.
Thanks, @EreMaijala!
This has been spun off from #2929. Makes the limit template similar to sort template.