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

fix/164: Show pagination when Sort by Order is clicked #165

Merged
merged 5 commits into from
Oct 5, 2023
Merged

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Sep 29, 2023

Closes #164

Description of the Change

In this PR, we additionally send posts_per_page as -1 when "Sort By Order" is clicked to enable pagination.

How to test the Change

Changelog Entry

Fixed - Hidden pagination in admin screen when Sort by Order is clicked

Credits

Props @tlovett1 @dkotter @Sidsector9

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@Sidsector9 Sidsector9 requested a review from a team as a code owner September 29, 2023 05:52
@Sidsector9 Sidsector9 requested review from ravinderk and removed request for a team September 29, 2023 05:52
@Sidsector9 Sidsector9 marked this pull request as draft September 29, 2023 05:52
@github-actions github-actions bot added this to the 2.6.0 milestone Sep 29, 2023
@github-actions github-actions bot added the needs:code-review This requires code review. label Sep 29, 2023
@github-actions github-actions bot removed the needs:code-review This requires code review. label Sep 29, 2023
@Sidsector9 Sidsector9 marked this pull request as ready for review September 29, 2023 09:51
@github-actions github-actions bot added the needs:code-review This requires code review. label Sep 29, 2023
Copy link
Collaborator

@dkotter dkotter left a comment

Choose a reason for hiding this comment

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

While this does appear to fix the pagination issue, what I'm seeing is it will no longer show the page heirarchy. For instance, if I have a parent page with multiple child pages, on the main list view, those are shown as nested beneath the parent. If I click on Sort by order prior to this fix, that nesting is still shown, though pagination doesn't show. With the changes in this PR, if I click on Sort by order, pagination now shows but the nesting heirarchy is lost

@Sidsector9
Copy link
Member Author

@dkotter that's a good catch. I took an alternative approach and tested it with child, as well as child of child elements. Can you take a look?

@Sidsector9 Sidsector9 requested review from dkotter and a team October 2, 2023 10:27
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

This will affect all queries on the page, not just the main query for the list table.

It's probably going to need:

			if ( ! $query->is_main_query() ) {
				return;
			}

* @param WP_Query $query The WP_Query instance (passed by reference).
*/
public static function filter_query( $query ) {
$is_simple_page_ordering = isset( $_GET['id'] ) ? 'simple-page-ordering' === sanitize_text_field( wp_unslash( $_GET['id'] ) ) : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to sanitize or unslash when comparing to a string that requires neither.

Suggested change
$is_simple_page_ordering = isset( $_GET['id'] ) ? 'simple-page-ordering' === sanitize_text_field( wp_unslash( $_GET['id'] ) ) : false;
// phpcs:ignore WordPress.Security.NonceVerification.Recommended
$is_simple_page_ordering = isset( $_GET['id'] ) ? 'simple-page-ordering' === $_GET['id'] : false;

simple-page-ordering.php Show resolved Hide resolved
@jeffpaul jeffpaul requested a review from peterwilsoncc October 4, 2023 13:24
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

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

LGTM and passes manual testing.

The E2E tests failed on the first run but it appears to have been a glitch as they passed on the second run.

@Sidsector9 Sidsector9 merged commit 1956eb6 into develop Oct 5, 2023
11 checks passed
@Sidsector9 Sidsector9 deleted the fix/164 branch October 5, 2023 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-review This requires code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pagination is hidden/disabled when "Sort by Order" is clicked
3 participants