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

Make page_count and current accessible from Twig #58

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

kylxbn
Copy link

@kylxbn kylxbn commented Mar 1, 2022

There are times when it is necessary to show the page_count and current page in Twig templates (for example, "2/6" to mean "page 2 of 6"). This pull request aims to expose variables current, items_per_page, and page_count.

I am not good with making pull requests, so if there is any mistake, please tell me, and I will try my best to fix it. Thanks!

There are times when it is necessary to show the page_count and current page in Twig templates (for example, "2/6" to mean "page 2 of 6"). This pull request aims to expose variables `current`, `items_per_page`, and `page_count`.

I am not good with making pull requests, so if there is any mistake, please tell me, and I will try my best to fix it. Thanks!
Copy link
Member

@mahagr mahagr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except that it would be nice to have at least docblocks to specify the return type. Though as we're on PHP 7.3+, int return type would be preferable.

I had to use `intval()` because `ceil()` returns a `double` by default, so I wanted to cast it to `int` first.
@kylxbn
Copy link
Author

kylxbn commented Mar 2, 2022

@mahagr , thank you very much for your feedback. I apologize for not adding docblocks in advance, because I wasn't sure which type the variables were in. However, after making sure that all are integers (after casting one double to int because ceil() returns a double), I added proper @return annotations.

I hope this is okay now. If there are any other mistakes, please tell me!

@mahagr
Copy link
Member

mahagr commented Mar 2, 2022

Looks good to me!

@kylxbn
Copy link
Author

kylxbn commented Mar 2, 2022

@mahagr , thank you very much for your patience. I originally intended to use casts but I was unsure, and the internet misguided me. I apologize.

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.

None yet

2 participants