-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 block: client-side pagination #53812
Conversation
Size Change: +313 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
Flaky tests detected in 0d87af1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5949163204
|
This looks good. It would be good to see #29484 revisited next for intra-post pagination, it was never properly done to begin with. |
…Enhanced Pagination" option is enabled
…ide navigation (#53853) * Add failing test * Fix the test * Add changelog * Fix lint error
* Add failing test * Fix test using key * Replace key with data-wp-key * Refactor test a bit * Add changelog * Add docs * Remove unnecessary paragraph * Fix lint error
…ion' into try/client-side-pagination-of-query-block
…vigation (#53876) * Add failing test * Fix the test
…ion' into try/client-side-pagination-of-query-block
Co-authored-by: David Arenas <darerodz@users.noreply.github.com>
Apparently, some screen readers don't read elements that don't have width and height.
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.
Code LGTM! It also behaves as expected. Approved! 🚀
PS: I'm aware there are still some issues to address (thanks, @pablohoneyhoney and @jeryj for the feedback 😊 ). We will focus on that in subsequent PRs.
As I noted in my previous comment on #53737, what is the plan for addressing these follow-ups? Memory is not good enough. This is how things get forgotten and go unaddressed. I am still trying to catch up after WCUS, haven't had time to review much. |
Commenting to support @alexstine here. Merging when it doesn't meet accessibility standards is merging in a broken state. It can take some time to make the mental shift, but please consider screen reader, keyboard, and other ATs as baselines rather than follow-ups. For example, I doubt this PR would have been merged if the pagination buttons didn't work via a mouse click. In the same way, it shouldn't have been merged without proper focus management. I'm not trying to be hard on y'all. I'm very glad you are all considering accessibility as important. I do want to get on the same page as to what is considered ready for merge or not though so we can have the same expectations. |
First of all, thanks a lot for your feedback and for sharing your concerns. I totally understand it, and I agree we should work on solving them. I had assumed that the noted accessibility issues were not as blocking and could be fixed in a subsequent PR before the coming release. Sorry for the rush to merge the PR; I should have spent more time on accessibility. 🙏 I already included them in the Tracking Issue, and I plan to work on them ASAP. I believe we can work on a PR to fix them during the RC to ensure it works fine before this Gutenberg release. 😊 |
Was there any discussion about putting this option in the Settings panel of the Pagination block? Not all queries include pagination, so finding the setting on the Query block itself might feel a bit strange in those circumstances. "User experience" is an ambiguous panel title, and generally we try to avoid ad hoc panels for narrow use cases. |
Yep, there's already a PR opened to solve that:
Uh, you're right. I think @luisherranz also noticed the same, as he suggested adding a pop-up to warn about activating the "enhanced pagination" when there's no Pagination block. It's mentioned under Tasks left for subsequent PRs, in the PR's description. @luisherranz, I guess we should move that task to the Tracking Issue, right?
Agree, and @pablohoneyhoney mentioned something similar in #53812 (comment). It is also addressed in #54198. |
We put it in the Query block because when this is activated, we need to add directives to the Query, Post Template and Pagination blocks. But if there's a way to set the block context in the Pagination block and read it in the Query and Post Template blocks, we'll happily move the setting there 🙂 |
True. I've just added it! |
What?
Built on top of #53733. Supersedes #44034.
This PR adds an option to the Query block to enhance the pagination with client-side navigation.
Why?
To improve the user experience.
How?
The pagination links are intercepted using a directive (
data-wp-on--click
) which prevents the regular server-side navigation and uses the Interactivity API'snavigate
function instead. It also prefetches the next/previous pages on mouseenter.Tasks
There are still many tasks that need to be done before this can be considered ready. I'll probably add more as we progress and find out what needs to be done.
supports.interactivity
and enqueue theview.js
file if the "Enhanced Pagination" option is enabled.Tasks left for subsequent PRs
Testing Instructions
You can use https://playground.wordpress.net/gutenberg.html to test this PR.
Screenshots or screencast
Screen.Capture.on.2023-08-18.at.15-12-54.mp4