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

DataViews Pagination: improve pagination logic to handle empty pages #68709

Closed
wants to merge 1 commit into from
Closed

DataViews Pagination: improve pagination logic to handle empty pages #68709

wants to merge 1 commit into from

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Jan 16, 2025 •
edited
Loading

What, Why and How?

This PR addresses an issue in the DataViewsPagination component where deleting the last item on a page results in a 'No results found' message instead of navigating to the previous page.

Changes Made:

  • Added logic to navigate to the previous page when the last item on the current page is deleted and there exists a previous page to navigate.

Testing Instructions

  1. Navigate to the site editor.
  2. Navigate to Patterns.
  3. Confirm all the patterns are displayed in a pagination.
  4. Make sure, on a particular page, that there's just one data item being displayed.
  5. Delete that particular data item.
  6. Confirm, that the page navigates back properly.

Screencasts

patterns-video.mov
templates-video.mov

Closes: #68707

@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Jan 16, 2025 •
edited
Loading

The failing test cases seems to be unrelated to the changes made in the PR.

@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review January 16, 2025 09:38
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 16, 2025 •
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: yogeshbhutkar <[email protected]>
Co-authored-by: Mayank-Tripathi32 <[email protected]>
Co-authored-by: Mamaduka <[email protected]>

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

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Jan 16, 2025
Mayank-Tripathi32
Mayank-Tripathi32 approved these changes Jan 16, 2025
View reviewed changes
Copy link
Contributor

@Mayank-Tripathi32 Mayank-Tripathi32 left a comment

Choose a reason for hiding this comment

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

Works for me! 👍

@yogeshbhutkar
Copy link
Contributor Author

Hi @Mamaduka, can you please review this PR when you get a moment?

Mamaduka
Mamaduka requested changes Jan 17, 2025
View reviewed changes
Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

I'm unsure about this "fix". Redirecting users because data for rendering is no longer available can have unexpected side effects.

The redirection can also cause a focus loss, which will regress a11y of this component.

When a user deletes all items on a page, displaying 'No results' messages seems correct since that page does not have any results. The current post/page list view (wp-admin/edit.php) behaves similarly.

packages/dataviews/src/components/dataviews-pagination/index.tsx
Comment on lines +33 to +41
// If the current page is not the first page, and there are no items on the current page, go to the previous page.
useEffect( () => {
if ( currentPage !== 1 && ! data.length ) {
onChangeView( {
...view,
page: currentPage - 1,
} );
}
}, [ data.length, currentPage, view, onChangeView ] );
Copy link
Member

Choose a reason for hiding this comment

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

This effect isn't reliable. What happens when you're not on the first page and data is fetching? The condition will be true, and the user will be redirected to the previous page.

P.S. I know the current page state doesn't persist in the URL, but that will change.

@yogeshbhutkar
Copy link
Contributor Author

yogeshbhutkar commented Jan 17, 2025 •
edited
Loading

When a user deletes all items on a page, displaying 'No results' messages seems correct since that page does not have any results.

I initially perceived this as a bug because the page number displayed in such cases is incorrect. Would it be more worthwhile to address the issue by correcting the page number display instead of implementing a redirection?

P.S. I know the current page state doesn't persist in the URL, but that will change.

Or maybe whenever this change is made, such a scenario can be factored in.

Dataviews.Pagination.Issue.68707.mov

@yogeshbhutkar
Copy link
Contributor Author

As mentioned in the previous comments, this might not be the intended behavior and might cause accessibility issues down the line. Moreover, the incorrect page number reflected when no results are found should be fixed when the page state persists in the URL.

For these reasons, it's better to close this PR. Please feel free to close the parent issue if it seems unrelated or unplanned.

@Mamaduka
Copy link
Member

Thanks, @yogeshbhutkar!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mamaduka Mamaduka Mamaduka requested changes

@Mayank-Tripathi32 Mayank-Tripathi32 Mayank-Tripathi32 approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

Assignees
No one assigned
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
No milestone
3 participants
@yogeshbhutkar @Mamaduka @Mayank-Tripathi32