Skip to content
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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

ThoWagen
Copy link
Contributor

@ThoWagen ThoWagen commented Nov 12, 2024

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

  • Update changelog when merging

Copy link
Member

@demiankatz demiankatz left a 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. :-)

@demiankatz
Copy link
Member

Also, just for the record, I ran the full test suite on my end and everything is passing.

@ThoWagen
Copy link
Contributor Author

ThoWagen commented Nov 13, 2024

@demiankatz I'm definitely open to modify this more but wanted to point out first that changes in Helpers/pagination.phtml are actually the current state in search/pagination.phtml.

Copy link
Member

@demiankatz demiankatz left a 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!

@demiankatz demiankatz added this to the 11.0 milestone Nov 13, 2024
@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture pull requests that involve significant refactoring / architectural changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants