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.

Sign up for GitHub

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

Jump to bottom

Pagination Numbers: Add data-wp-key to pagination numbers if enhanced pagination is enabled #58189

Merged
merged 2 commits into from
Feb 12, 2024
Merged

Pagination Numbers: Add data-wp-key to pagination numbers if enhanced pagination is enabled #58189

merged 2 commits into from
Feb 12, 2024

Conversation

DAreRodz
Copy link
Contributor

@DAreRodz DAreRodz commented Jan 24, 2024 •
edited
Loading

What?

Adds the data-wp-key directive to the elements rendered by the Pagination Numbers block to prevent issues after client-side navigations. This is done only when the "enhanced pagination" is enabled.

Why?

Currently, after a couple of navigations, numbers start to turn into ellipsis characters due to the preact library having problems reconciling the new HTML with the current one.

How?

Using each tag's position index to generate the key instead of a unique ID for each link. I know it's not appropriate to use data-wp-key, but it seems to be the only way to prevent text nodes between numbers from disappearing. If those are removed, the numbers "collapse" as there is no space in between. (Preact seems to remove the text nodes in between while re-ordering tags as text nodes don't have any key).

No fix Unique ID Tag Index
Screen.Recording.2024-02-12.at.15.27.59.mov
Screen.Recording.2024-02-12.at.15.19.19.mov
Screen.Recording.2024-02-12.at.15.20.00.mov

Testing Instructions

  1. In the site editor, go to the home template and disable "Force page reload" in the Query block.
  2. Visit the homepage.
  3. Navigate using the pagination links.
  4. Ensure no duplicated or extra numbers appear in the pagination numbers.
  5. Ensure links work as expected.

@DAreRodz DAreRodz added [Type] Bug An existing feature does not function as intended [Block] Query Pagination Affects the Query Pagination Block - used for pagination within the Query Loop Block labels Jan 24, 2024
@luisherranz luisherranz mentioned this pull request Feb 1, 2024
Closed
47 tasks
@DAreRodz DAreRodz force-pushed the fix/query-pagination-numbers-data-wp-keys branch from ab761aa to 3255e10 Compare February 12, 2024 11:34
@DAreRodz DAreRodz force-pushed the fix/query-pagination-numbers-data-wp-keys branch from 986a90b to f61a275 Compare February 12, 2024 14:15
@DAreRodz DAreRodz marked this pull request as ready for review February 12, 2024 14:31
@DAreRodz DAreRodz requested a review from ajitbohra as a code owner February 12, 2024 14:31
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 12, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

SantosGuillamot
SantosGuillamot reviewed Feb 12, 2024
View reviewed changes
packages/block-library/src/query-pagination-numbers/index.php
) ) {
$p->set_attribute( 'data-wp-on--click', 'core/query::actions.navigate' );
if ( null === $p->get_attribute( 'data-wp-key' ) ) {
$p->set_attribute( 'data-wp-key', 'index-' . $tag_index++ );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use the $page variable instead of creating our own tag_index?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, looking at the code:

$page_key = isset( $block->context['queryId'] ) ? 'query-' . $block->context['queryId'] . '-page' : 'query-page';
$page = empty( $_GET[ $page_key ] ) ? 1 : (int) $_GET[ $page_key ];

As far as I can remember, the queryId only appears when there is more than one query loop in the page. So, in most cases, $page will always return 1, having this HTML
Screenshot 2024-02-12 at 16 22 46

With the same key, the fix won't work.

SantosGuillamot
SantosGuillamot approved these changes Feb 12, 2024
View reviewed changes
Copy link
Contributor

@SantosGuillamot SantosGuillamot left a comment

Choose a reason for hiding this comment

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

It seems to be working fine for me 👍

I just left a small comment and I don't fully understand the implications of using data-wp-key

@cbravobernal cbravobernal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 12, 2024
@cbravobernal cbravobernal merged commit f1b8ebb into trunk Feb 12, 2024
66 of 67 checks passed
@cbravobernal cbravobernal deleted the fix/query-pagination-numbers-data-wp-keys branch February 12, 2024 15:28
@github-actions github-actions bot added this to the Gutenberg 17.8 milestone Feb 12, 2024
aristath added a commit that referenced this pull request Feb 13, 2024
@aristath
Merge branch 'trunk' into try/fix-form-styles
f612d4e 
* trunk: (273 commits)
  Remove preffered style variations legacy support (#58930)
  Style theme variations: add property extraction and merge utils (#58803)
  Migrate `change-detection` to Playwright (#58767)
  Update Changelog for 17.6.6
  Docs: Clarify the status of the wp-block-styles theme support, and its intent (#58915)
  Use `data_wp_context` helper in core blocks and remove `data-wp-interactive` object (#58943)
  Try double enter for details block. (#58903)
  Template revisions API: move from experimental to compat/6.4 (#58920)
  Editor: Remove inline toolbar preference (#58945)
  Clean up link control CSS. (#58934)
  Font Library: Show error message when no fonts found to install (#58914)
  Block Bindings: lock editing of blocks by default (#58787)
  Editor: Remove the 'all' rendering mode (#58935)
  Pagination Numbers: Add `data-wp-key` to pagination numbers if enhanced pagination is enabled (#58189)
  Close link preview if collapsed selection when creating link (#58896)
  Fix incorrect useAnchor positioning when switching from virtual to rich text elements (#58900)
  Upgrade Floating UI packages, fix nested iframe positioning bug (#58932)
  Site editor: fix start patterns store selector (#58813)
  Revert "Rich text: pad multiple spaces through en/em replacement (#56341)" (#58792)
  Documentation: Clarify the performance reference commit and how to pick it (#58927)
  ...
youknowriad pushed a commit that referenced this pull request Feb 13, 2024
@DAreRodz @SantosGuillamot
@cbravobernal @youknowriad
Pagination Numbers: Add data-wp-key to pagination numbers if enhanc…
f7e9fdf 
…ed pagination is enabled (#58189)

* Add `data-wp-key` to pagination numbers

* Use tag index as the `data-wp-key` value

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org>
@youknowriad
Copy link
Contributor

I just cherry-picked this PR to the backports/beta1 branch to get it included in the next release: f7e9fdf

@youknowriad youknowriad removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 13, 2024
@youknowriad youknowriad mentioned this pull request Feb 13, 2024
Merged
youknowriad pushed a commit that referenced this pull request Feb 13, 2024
@DAreRodz @SantosGuillamot
@cbravobernal @youknowriad
Pagination Numbers: Add data-wp-key to pagination numbers if enhanc…
ed5f56e 
…ed pagination is enabled (#58189)

* Add `data-wp-key` to pagination numbers

* Use tag index as the `data-wp-key` value

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org>
youknowriad pushed a commit that referenced this pull request Feb 13, 2024
@DAreRodz @SantosGuillamot
@cbravobernal @youknowriad
Pagination Numbers: Add data-wp-key to pagination numbers if enhanc…
333aee8 
…ed pagination is enabled (#58189)

* Add `data-wp-key` to pagination numbers

* Use tag index as the `data-wp-key` value

Co-authored-by: DAreRodz <darerodz@git.wordpress.org>
Co-authored-by: SantosGuillamot <santosguillamot@git.wordpress.org>
Co-authored-by: c4rl0sbr4v0 <cbravobernal@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SantosGuillamot SantosGuillamot SantosGuillamot approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@cbravobernal cbravobernal Awaiting requested review from cbravobernal

Assignees
No one assigned
Labels
[Block] Query Pagination Affects the Query Pagination Block - used for pagination within the Query Loop Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 17.8
Development

Successfully merging this pull request may close these issues.
4 participants
@DAreRodz @youknowriad @SantosGuillamot @cbravobernal