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

Query loop: the 'Max page to show' setting doesn't preview in the editor #63027

Open
afercia opened this issue Jul 1, 2024 · 6 comments
Open
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 1, 2024

Description

Discovered while working on #63020

The editor is supposed to provide a preview of the blocks (the entire page, I would say) that is as close as possible to the page on the front end. However, it appears the Query loop 'Max page to show' setting doesn't provide any preview in the editor. It just stays in its default view.

Not sure but this may happen because the Paginaion is actually a separate block. Regardless, as a user I'd be pretty disoriented by not seeing any change in the editor preview and think there's something not working as expected.

Step-by-step reproduction instructions

  • Create a new post.
  • Add a Query loop block > Start blank > Title & date.
  • The editor will fetch some post titles and dates and will display them in the editor UI. Screenshot:

Screenshot 2024-07-01 at 15 41 04

  • Observe the Pagination link show Previous, all the pages link (with an ellipsis if there's a lot of pages) and the Next links.
  • In the block toolbar, click 'Display settings'.
  • Note: in Make usage of the settings icon more consistent #63020 these settings will be moved to the Inspector panel.
  • Change Items per page and observe the editor does update the preview accordingly.
  • Change Offset and observe the editor does update the preview accordingly.
  • Change 'Max page to show' to, for example, 2 and observe the editor does not update the preview. Screenshot:

Screenshot 2024-07-01 at 15 41 32

  • Save and go to the front end. Observe the pagination actually shows only two pagination links. Screenshot:

Screenshot 2024-07-01 at 15 42 47

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Block] Query Loop Affects the Query Loop Block labels Jul 1, 2024
@ankur0007
Copy link

Hi Mate, I am looking into this and let you know if I can fix it :)

@ankur0007
Copy link

ankur0007 commented Jul 4, 2024

Hi,

I've reviewed it, and here are my thoughts:

  1. When clicking on pagination numbers, there's a setting in a separate block called "Number Of Links." This setting determines how many numbers will appear before and after the current pagination number. But this is not a solution.

  2. To dynamically change the pagination number, we must fetch the count of posts from the selected post type when initializing the query loop block. Depending on the parameters selected—like Items per page, Offset, and Max pages to show—we'll dynamically generate the appropriate number of pagination links and replace them with the current ones.

Regarding 2, do you think implementing this will result in additional fetch requests to get the post count?

Regards,
A. V.

@afercia
Copy link
Contributor Author

afercia commented Jul 8, 2024

do you think implementing this will result in additional fetch requests to get the post count?

It's a reasonable concern but this already happens when changing the 'Items per page' and 'Offset' values. At each change, posts are fetched including all the posts content, which may be potentially huge. As far as I see, the responses are cached so that subsequent requests with the same values won't trigger a new fetch.

On the other hand, triggering a new request at any React onChange event doesn't seem to be ideal. Especially when changing the values by using the number input arrows button or by using the keyboard. Say I want to change 'Items per page' form 10 to 20. That would trigger 10 requests each time the value changes.
It's an UI / interaction issue. This kind of UIs that trigger requests should be implemented by 'debouncing' the request. WordPress Core does that in a few places:

  • While the user is changing the valaues, debounce the request with a reasonable value. Generally core uses 500ms, I think.
  • Trigger the request only after the user stops changing the value, after 500ms.

Debouncing should be a standard in these cases. Cc @youknowriad any thoughts?
I guess it's worth creating a new issue.

@ntsekouras
Copy link
Contributor

Thanks for the issue, but I don't think it's something we should do, as the decision to render pagination blocks like that (static) was intentional.

While your example seems fine to had that change in pagination, it's not worth it to add that much complexity and make extra requests(performance), as in most cases users just need to have a way of changing settings in pagination blocks and style them.

I suggest we close this one.

@afercia
Copy link
Contributor Author

afercia commented Nov 4, 2024

Thanks for the issue, but I don't think it's something we should do, as the decision to render pagination blocks like that (static) was intentional.

@ntsekouras thanks for your feedback. I'm not sure I understand how that relates to whether it was a good or not so good decision. I appreciat eit was intentional but thtat doesn't necessarily mean it was a good decision.

it's not worth it to add that much complexity and make extra requests(performance), as in most cases users just need to have a way of changing settings in pagination blocks and style them.

I don't think this would add 'complexity' can you please elaborate on your concerns in that regard?

I don't share the concern about performance. First, as mentioned in this issue description, changing other settings like 'Items per page' or 'Offset' already triggers a new request. Changing 'Max page to show' wouldn't be different so your concerns aout performance should theoretically apply to all settings, not just this specific case. Secondarily, ideally, any request should also return 'meta information' in the response like the classic WP_Query does with found_posts. When 'found_posts' is known, it would then be just a calculation.

However, I'm not even sure that would be needed. The editor is supposed to provide a visual preview of the front end. The pagination linnks preview in the editor are non-functional links. They are just a preview. When changing the 'Max pages to show' setting, the Pagination block should only change how many links are visually shown in the preview. Not sure there's a need of triggering a new request in the first place?

in most cases users just need to have a way of changing settings in pagination blocks and style them.

Yes, and in order to do so users need a 1-to-1 preview of the front end. To me, this seems a valid issue and closing it would go against the whole purpose of providing users with an accurate preview of the front end.

@ntsekouras
Copy link
Contributor

ntsekouras commented Nov 5, 2024

I don't think this would add 'complexity' can you please elaborate on your concerns in that regard?

If I'm not wrong this will require implementing a close clone of paginate_links in JS. Maybe it's simpler than what I have in mind, but I don't think so.

Regarding the performance, yeah, it will not be a bad hit.

In general I think it's not great to do this in this dynamic block. For example we create a Query Loop block when we have just two results. That might not even show any pagination blocks probably and after a while the user adds more posts and has to go back to the post with the Query Loop block and style accordingly the pagination blocks, based on the number of results at that specific time.

I appreciat eit was intentional but thtat doesn't necessarily mean it was a good decision.

Sure. I'm just sharing my thoughts as I implemented these blocks initially, and of course people can chime in and change that eventually. Let's keep it open to gather more feedback then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants