-
Notifications
You must be signed in to change notification settings - Fork 355
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
Refactor pagination #4074
base: dev
Are you sure you want to change the base?
Refactor pagination #4074
Conversation
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'm interested in @EreMaijala's opinion on this, since he's refactored some of this code more recently than I have, but it seems pretty reasonable based on my first review. See below for some minor questions and comments.
Regarding whether to include improved responsive design in the core project, I'm certainly open to that, but I think it would make sense to do it as a separate follow-up PR so we don't tie up the basic infrastructure improvements if there end up being extended discussions about details of the aesthetics. :-)
Also, just for the record, I ran the full test suite on my end and everything is passing. |
@demiankatz I'm definitely open to modify this more but wanted to point out first that changes in |
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.
Since all of my previous observations turned out not to be relevant, I think this is probably ready to go as-is. But I'll wait and see what @EreMaijala has to say before I finalize the merge!
In this PR the pagination templates are refactored to reduce code duplication and by adding extra classes to some elements.
We want to use those classes to improve the responsive design. On small screens the number of selectable pages shall be reduced and the text on the "Next" and "Prev" buttons removed. If you believe that should be part of the standard VuFind themes, I can also add this here.
TODO