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

Page parameter name #820

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

steveWinter
Copy link

This PR resolves #815 in the manner suggested by @garak in that issue - passing the entire SlidingPagination object to the templates to allow the pageParameterName to be accessed.

In the future it would likely be possible to remove some of the local variables which are created as the template is being rendered since they could be accessed directly from the SlidingPagination, however I wanted to keep this PR to solving just the issue at hand.

@garak
Copy link
Collaborator

garak commented Feb 15, 2025

Do we really need to pass the entire Pagination object? I think passing only the options array can be enough.

@steveWinter
Copy link
Author

steveWinter commented Feb 16, 2025

Hi @garak in this comment on the issue you said

Probably we should pass the entire set of options to the PaginationRuntime constructor, while currently we pass only a few.

Did I miss-understand what you had meant? Or not implement it in the way you expected?

Essentially I took the code you proposed, updated it slightly for changed variable names, then modified the templates to pass in that third parameter.

@garak
Copy link
Collaborator

garak commented Feb 16, 2025

Well, I wrote that code a few months ago. Probably I would ask the same question to the past myself: why passing the entire object, when we can pass only the options? Moreover, considering that the object is only used to retrieve the options themselves.

@steveWinter
Copy link
Author

steveWinter commented Feb 16, 2025

I believe your rationale was

I'm sure it works, but it would force use to extract the pageParameterName to be passed everywhere. Moreover, it's not future-proof: if one day we find in the need of another option, we would be forced to add a new argument.

How would you like me to proceed? I can modify it such that we pass only that parameter (which for reference is how is used to work in v5), or we can continue with this approach?

@garak
Copy link
Collaborator

garak commented Feb 17, 2025

Change the method to accept an array, pass $pagination->getOptions(), access the page parameter name from the options array.

@steveWinter
Copy link
Author

@garak thank you for your feedback - I've pushed another commit which I hope as implemented things as you intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pageParameterName not supported
2 participants