-
Notifications
You must be signed in to change notification settings - Fork 0
Update pagination buttons to include results count #275
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
Conversation
Why are these changes being introduced: * UX design includes showing the number of results that will be shown when clicking the Next or Previous pagination buttons. * Previous page should always be the same number of results as per_page so it is simpler to say "Previous 20 results" rather than trying to calculate how many results remain before the start index. * Next page should show the remaining results if fewer than per_page results remain. * Also introduces a disabled state via a span for the next and previous buttons when there are no more results in that direction. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/USE-184 How does this address that need: * Mathed out the new pagination button text requirements * Updated PaginationHelper methods to calculate remaining results * Updated PaginationHelper methods to render disabled span when no more results in that direction
Pull Request Test Coverage Report for Build 19371564204Details
💛 - Coveralls |
jazairi
left a comment
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.
This looks good and works as intended. I'm a bit reticent to to hardcode the number of previous results in the helper, since it's possible that value could change over time. If you wouldn't mind making that change, then I think this is ![]()
Also, I noticed that the spinner weirdly isn't showing up anymore, but it's unrelated to this changeset because it's also happening in staging. I'll check in on Slack before ticketing.
app/helpers/pagination_helper.rb
Outdated
| end | ||
|
|
||
| def prev_page_label | ||
| 'Previous 20 results' |
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'd prefer this be set to @pagination[:per_page], as the actual value will vary depending on what RESULTS_PER_PAGE is set to in the Analyzer model. (We should probably make that an env var, but that's neither here nor there.)
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.
Doh. Fully agree!
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.
Updated, thanks for catching that! Also incorporated a change requested by Dave to use Previous 0 for the first page results.
NOTE: you can see the end state logic using the search
railsin the timdex tabWhy are these changes being introduced:
Relevant ticket(s):
How does this address that need:
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing