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

Pagination with router params #4698

Merged
merged 28 commits into from
Jun 9, 2023
Merged

Conversation

erral
Copy link
Member

@erral erral commented Apr 14, 2023

Fixes #3868 and #4692

@netlify
Copy link

netlify bot commented Apr 14, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit e04278e
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/64390d761709060008f9ef2e

@netlify
Copy link

netlify bot commented Apr 14, 2023

Deploy Preview for volto ready!

Name Link
🔨 Latest commit 54e55e1
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/6482c8f9732df200088e5897
😎 Deploy Preview https://deploy-preview-4698--volto.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cypress
Copy link

cypress bot commented Apr 14, 2023

Passing run #5489 ↗︎

0 503 20 0 Flakiness 0

Details:

Merge branch 'master' into pagination-with-router-params
Project: Volto Commit: 54e55e16a9
Status: Passed Duration: 15:13 💡
Started: Jun 9, 2023 6:42 AM Ended: Jun 9, 2023 6:57 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@erral erral force-pushed the pagination-with-router-params branch from c7b9845 to b35f5d1 Compare April 20, 2023 09:37
@erral erral closed this Apr 20, 2023
@erral erral reopened this Apr 20, 2023
@erral
Copy link
Member Author

erral commented Apr 20, 2023

@sneridagh we have reworked this PR and added cypress tests to show that the ?page=XX functionality is working. We test clicking on the pagination numbers, going back, reloading the page and going to some other page too.

In the meantime we have fixed the withQuerystringResults function, which wasn't checking for the request being loaded, but being "not loading" which created additional queries to the endpoint.

@erral erral marked this pull request as ready for review April 20, 2023 13:22
@erral erral requested a review from sneridagh April 20, 2023 13:23
@sneridagh sneridagh requested review from reebalazs and robgietema May 6, 2023 09:13
@sneridagh
Copy link
Member

sneridagh commented May 6, 2023

@erral @ionlizarazu I've found a problem, see video. When I create a page, then go back to the listing, it's still empty :( so the query is not being triggered on route change for some reason. I've tried on demo and works well.

Screen.Recording.2023-05-06.at.11.23.48.mov

@ionlizarazu
Copy link
Contributor

@erral @ionlizarazu I've found a problem, see video. When I create a page, then go back to the listing, it's still empty :( so the query is not being triggered on route change for some reason. I've tried on demo and works well.

Screen.Recording.2023-05-06.at.11.23.48.mov

I've added a cypress test for this and fixed it.

@robgietema
Copy link
Member

How does this work when I add multiple listing blocks on 1 page?

@ionlizarazu
Copy link
Contributor

How does this work when I add multiple listing blocks on 1 page?

https://cloud.cypress.io/projects/hvviu4/runs/df7a2ee0-472a-456a-8191-814a1b6c3b35/test-results/5fa7949f-3218-4883-a052-3adf784c06d4/video

* @returns {string} Example: page || page_012345678
*/
const useCreatePageQueryStringKey = (id) => {
const blockTypesWithPagination = ['search', 'listing'];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should come from the config so that addon blocks can use pagination.

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean the 'blockTypesWithPagination'?

Copy link
Member

Choose a reason for hiding this comment

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

@ionlizarazu Yes. But it can wait for a future PR, rather than delaying this one.

@davisagli
Copy link
Member

I tried editing a search block and changing the number of items per page, then saving. The items shown per page updated correctly in edit mode, but after I saved, the view mode was still showing the old number of items per page until I reloaded.

parseInt(
qs.parse(location.search)?.[pageQueryStringKey] || defaultPage,
),
);
Copy link
Member

Choose a reason for hiding this comment

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

The parsing of the page from the querystring needs to handle a couple edge cases better:

  1. I navigated to ?page=x and the pagination showed NaN
  2. I navigated to ?page=-4 and the block showed no results.

If the value isn't a positive integer, this should probably just use the default page.

@ionlizarazu
Copy link
Contributor

I tried editing a search block and changing the number of items per page, then saving. The items shown per page updated correctly in edit mode, but after I saved, the view mode was still showing the old number of items per page until I reloaded.

This is not an issue we are handling in this PR and it happens in the actual Volto version. Can we manage it in another issue or PR?

@davisagli
Copy link
Member

I tried editing a search block and changing the number of items per page, then saving. The items shown per page updated correctly in edit mode, but after I saved, the view mode was still showing the old number of items per page until I reloaded.

This is not an issue we are handling in this PR and it happens in the actual Volto version. Can we manage it in another issue or PR?

Yes, if it is not a regression from this PR, we can handle it separately.

@ionlizarazu
Copy link
Contributor

Can we merge this?

@sneridagh
Copy link
Member

@ionlizarazu I also think it's ready, I'll try to merge it this week.

@sneridagh sneridagh merged commit b025c50 into master Jun 9, 2023
@sneridagh sneridagh deleted the pagination-with-router-params branch June 9, 2023 10:46
sneridagh pushed a commit that referenced this pull request Jun 23, 2023
Co-authored-by: bipoza <bipoza@gmail.com>
Co-authored-by: ionlizarazu <ilizarazu@codesyntax.com>
Co-authored-by: Unai <uetxaburu@codesyntax.eus>
sneridagh added a commit that referenced this pull request Jun 23, 2023
Co-authored-by: Mikel Larreategi <mlarreategi@codesyntax.com>
Co-authored-by: bipoza <bipoza@gmail.com>
Co-authored-by: ionlizarazu <ilizarazu@codesyntax.com>
Co-authored-by: Unai <uetxaburu@codesyntax.eus>
sneridagh added a commit that referenced this pull request Jun 25, 2023
* master:
  Release 17.0.0-alpha.14
  Linked headlines (#3540)
  Release notes for 16.20.8 16.21.0 16.21.1 (#4910)
  Spanish translation (#4896)
  Refactor Anontools (#4845)
  Update to plone-backend 6.0.5 (#4897)
  Release 17.0.0-alpha.13
  Enforce max upload size (#4868)
  Fix and improve the `addStyling` helper (#4880)
  Release 17.0.0-alpha.12
  Fix regression in horizontal scroll in contents view, add it back (#4872)
  Configurable Container component from registry for some key route views. (#4871)
  Allow to deselect color in ColorPickerWidget. (#4839)
  Release 17.0.0-alpha.11
  Pagination with router params (#4698)
  Release 17.0.0-alpha.10
  feat(slate): Add css identifier to slate style menu options (#4847)
  Update Brazilian Portuguese translations (Fixes #4853)
  Convert header class to function (#4767)
sneridagh added a commit that referenced this pull request Jun 26, 2023
* master: (29 commits)
  Remove anonymous function calls. Remove default exports from. (#4917)
  Release 17.0.0-alpha.14
  Linked headlines (#3540)
  Release notes for 16.20.8 16.21.0 16.21.1 (#4910)
  Spanish translation (#4896)
  Refactor Anontools (#4845)
  Update to plone-backend 6.0.5 (#4897)
  Release 17.0.0-alpha.13
  Enforce max upload size (#4868)
  Fix and improve the `addStyling` helper (#4880)
  Release 17.0.0-alpha.12
  Fix regression in horizontal scroll in contents view, add it back (#4872)
  Configurable Container component from registry for some key route views. (#4871)
  Allow to deselect color in ColorPickerWidget. (#4839)
  Release 17.0.0-alpha.11
  Pagination with router params (#4698)
  Release 17.0.0-alpha.10
  feat(slate): Add css identifier to slate style menu options (#4847)
  Update Brazilian Portuguese translations (Fixes #4853)
  Convert header class to function (#4767)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Pagination component does not keep state when navigating away from it
7 participants