-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 mid size parameter settable for Query Pagination block. #51216
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @krokodok! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Any ideas why the two tests are failing? |
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.
Thanks for the PR @krokodok ! Great work!
Unfortunately this PR fell into the cracks for a while. Do you think you'll find some time to rebase and move this forward?
I updated my code. Any feedback is again highly welcome, so that we can get this feature into the main trunk. I would also highly appreciate some explanations for all the failing unit tests. I can't understand what PHPCS does not like in the test |
Ok never mind the PHP formatting, I had an incomplete commit there. |
Thanks for updating the PR. The control works well, but I don't see the result on the frontend? Regardless of the value, I always see the same number of page numbers. I'm also not sure |
The last partial commit corrupted the php file again. My new commit should fix it. I don't think it hurts to have the 0. It will result in a pagination like this: It all depends on what the developer / designers have in mind. This is where my incentive to add this parameter control stems from in the first place: Being frustrated that I had to write custom PHP code just to change the number of pagination links that are visible. |
Perhaps it's just the Changing the help text might eliminate some confusion:
What do you think? |
I updated the help description to:
You are right, the whole concept of "midSize" takes a little math to understand. This control should not lead to more confusion than before. |
I think this may need a rebase. |
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.
Thank you for following up with this! 🚀 This is really close and we can take this to the finish line. I've left some small comments and there are some formatting issues to be fixed.
In my testing this worked well, but I didn't test the case where the query inherits(see the related comment below).
...ages/block-library/src/query-pagination-numbers/query-pagination-numbers-mid-size-control.js
Outdated
Show resolved
Hide resolved
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.
Thanks so much for iterating @krokodok! I left a super tiny comment, but I've tested everything and LGTM! Great work!
What you also need is to run npm run fixtures:regenerate
to update the integration/fixtures/blocks/core__query-pagination-numbers.json
file. After that the unit tests should pass.
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.
Thank you, great work! 🚀
Thank you so much for your help on this everybody. :) |
What?
This PR adds an input to the Query Pagination block controlling the
mid_size
parameter of the frontend renderedpaginate_links()
function. Up until now, only the default of2
formid_size
could be rendered with the Query Pagination block.This is my first try to contribute to Gutenberg at all. I am also very new to JSX. I took heavy inspiration for this feature and PR from #50779 which was merged just 4 days ago. Any feedback is welcome!
Why?
Closes #45855. It would be very helpful because up until now, developers had to control the amount of pagination numbers via PHP filters or with JS.
How?
The PR introduces a new attribute
mid_size
with a default of2
, which is also the default of the calledpaginate_links()
function in the frontend. The input is only shown if a Query Page Numbers block is present inside the Query Pagination block.Testing Instructions
mid_size
of2
is applied. The Query Page Numbers block should read:1 2 3 4 5 … 6
.mid_size
. For example, amid_size
of0
should result in1 … 2
and amid_size
of4
should result in1 2 3 4 5 6 7 8 9 … 10
.10
.mid_size
control should only be visible if a Query Page Numbers block is present. Delete the Query Page Numbers block and see, that themid_size
control is gone on the Query Pagination block.Testing Instructions for Keyboard
mid_size
control and change the number using the arrow-up and arrow-down keys or input a number using a number key. See, that the number of pages in the Query Page Numbers block immediately adjusts.10
.mid_size
control is gone.Front end testing instructions:
mid_size
to a low number like0
or1
and ensure that the rendered number of page links in the frontend is correct.mid_size
to a high number like3
or4
and ensure that the rendered number of page links in the frontend is correct.paginate_links()
function should ensure correct behaviour.Screenshots or screencast
Bildschirmaufnahme.2023-06-03.um.23.00.35.mov