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

Add current page parameter to the route in the listing and search block pagination #4159

Merged
merged 32 commits into from
Apr 5, 2023

Conversation

bipoza
Copy link
Contributor

@bipoza bipoza commented Dec 21, 2022

Added the current page parameter to the route in the listing and search block pagination.

Fixes: #3868

Demo with listing block

volto_listing_blcok_pagination.mov

Demo with search block

volto_pagination_with_searchblock.mov

Demo combining multiple lists

volto-multiple-blocks-with-pagination.mov

QueryString compatibility:

To be able to combine the searchBlock with the pagination parameters, we had to change the parameters of the searchBlock from hash to search (# -> ?).

We are contemplating the possibility of maintaining both types of routes so as not to generate a breaking change.

In general the compatibility is good except for a small error in the design of the SearchInput. With the old hash url, the clear icon does not render well, although the functionality is correct.

search_block_with_hash_compatibility.mov

@netlify
Copy link

netlify bot commented Dec 21, 2022

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit 8b264cb
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/642d810159f71500089f0c86

@cypress
Copy link

cypress bot commented Dec 21, 2022

Passing run #4926 ↗︎

0 489 20 0 Flakiness 0

Details:

Merge branch 'master' into pagination-with-router-params
Project: Volto Commit: e04278e86f
Status: Passed Duration: 14:49 💡
Started: Apr 14, 2023 8:26 AM Ended: Apr 14, 2023 8:41 AM

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

@bipoza bipoza changed the title WIP: Added current page parameter to route in listing and search block WIP: Added the current page parameter to the route in the listing and search block pagination Dec 21, 2022
@bipoza bipoza requested a review from erral December 21, 2022 18:46
@ionlizarazu
Copy link
Contributor

The bug you mention exists in the current (16.4.0) Volto version

@erral erral changed the title WIP: Added the current page parameter to the route in the listing and search block pagination Added the current page parameter to the route in the listing and search block pagination Dec 21, 2022
@erral erral changed the title Added the current page parameter to the route in the listing and search block pagination Add current page parameter to the route in the listing and search block pagination Dec 21, 2022
@sneridagh
Copy link
Member

@bipoza @erral wow! thanks for working in this. I know is specially difficult to accomplish. How are we maintaining the sync with the state-URL? I know this is a major pain point. Could we have unit tests for this feature? How about acceptance tests?

What we are trying to accomplish here is quite complex and I'd like to discuss this in the next Volto Team meeting with the team, and specially @tiberiuichim who tried to implement this in the past.

I'd like to get this well, specially agree on the format and serialization of the URL query parameters, because if we have to change them in the future for whatever reason, it will force a breaking change (people having saved URLs as bookmarks, etc)

@erral
Copy link
Member

erral commented Mar 24, 2023

We have seen that we have a bug in the search block with this PR.

We are preparing a fix and some tests to improve the PR.

@@ -24,26 +27,29 @@ const useCreatePageQueryStringKey = (id) => {
* A pagination helper that tracks the query and resets pagination in case the
* query changes.
*/
export const usePagination = (id = null, defaultPage = 1) => {
export const usePagination = (blockId = null, defaultPage = '1') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd think we should stick to a generic id rather then blockId and a number for the defaultPage.

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 "blockId" naming or something else?

@ionlizarazu
Copy link
Contributor

We have written some tests for the usePagination.js file

@erral
Copy link
Member

erral commented Apr 2, 2023

@sneridagh we think this is ready

@sneridagh sneridagh merged commit c40e772 into master Apr 5, 2023
@sneridagh sneridagh deleted the pagination-with-router-params branch April 5, 2023 15:56
sneridagh added a commit that referenced this pull request Apr 5, 2023
* master: (67 commits)
  Add current page parameter to the route in the listing and search block pagination (#4159)
  Fixed wrong localization on password reset page(#4656) (#4657)
  Release notes for 16.19.0 (#4655)
  Razzle upgrade notice in upgrade guide (#4641)
  Release generate-volto 7.0.0-alpha.3
  Update to latest Razzle - needed since #3997. This fixes the duplicated Razzles issue (#4640)
  3092 improve spellcheck (#4633)
  developer process for first time contributing (#4617)
  Trigger CI on pull_request event (#4629)
  Pining of `pydata-sphinx-theme` and `sphinx-book-theme`, CI is complaining. (#4626)
  Set sameSite in `18N_LANGUAGE` cookie (#4627)
  Update simple-git (#4546)
  DefaultView (blocks disabled): Show field name as tip on hover of label (#4598)
  Fix regexp that checks valid URLs and improve tests (#4601)
  Release 17.0.0-alpha.3
  Update changelog relese 16.18.0 (#4596)
  InternalURl helper method should incorporate externalRoutes settings … (#4560)
  Update message Add-on control panel (#4574)
  Fix video warnings link errors (#4578)
  Using Vale in CI for spellcheck (#4423)
  ...
sneridagh added a commit that referenced this pull request Apr 10, 2023
* master: (326 commits)
  Make URL a literal string to fix broken link (#4667)
  Move developer guidelines to contributing #4665 (#4666)
  Update Volto contributing to align with and refer to the new Plone co… (#4634)
  Release @plone/scripts 3.0.0
  Changelog
  Added querystring search get option (#4658)
  Add current page parameter to the route in the listing and search block pagination (#4159)
  Fixed wrong localization on password reset page(#4656) (#4657)
  Release notes for 16.19.0 (#4655)
  Razzle upgrade notice in upgrade guide (#4641)
  Release generate-volto 7.0.0-alpha.3
  Update to latest Razzle - needed since #3997. This fixes the duplicated Razzles issue (#4640)
  3092 improve spellcheck (#4633)
  developer process for first time contributing (#4617)
  Trigger CI on pull_request event (#4629)
  Pining of `pydata-sphinx-theme` and `sphinx-book-theme`, CI is complaining. (#4626)
  Set sameSite in `18N_LANGUAGE` cookie (#4627)
  Update simple-git (#4546)
  DefaultView (blocks disabled): Show field name as tip on hover of label (#4598)
  Fix regexp that checks valid URLs and improve tests (#4601)
  ...
sneridagh added a commit that referenced this pull request Apr 11, 2023
…ck pagination (#4159)

Co-authored-by: Mikel Larreategi <mlarreategi@codesyntax.com>
Co-authored-by: Víctor Fernández de Alba <sneridagh@gmail.com>
Co-authored-by: ionlizarazu <ilizarazu@codesyntax.com>
sneridagh added a commit that referenced this pull request Apr 13, 2023
sneridagh added a commit that referenced this pull request Apr 14, 2023
sneridagh added a commit that referenced this pull request Apr 14, 2023
sneridagh added a commit that referenced this pull request Apr 17, 2023
* master: (22 commits)
  Release changelog notes for 16.20.1
  Release 17.0.0-alpha.5
  Generate a split sitemap (also fix robots.txt) (#4639)
  Fix search block in edit mode re-queries multiple blocks with an empty search text (#4694)
  Fix Move to top of folder ordering in folder content view (#4691)
  Changelog
  Revert "Add current page parameter to the route in the listing and search block pagination (#4159)" (#4695)
  Release generate-volto 7.0.0-alpha.4
  Force the resolution of the `react-error-overlay` package to `6.0.9` (#4687)
  Fix training links (#4635)
  Release 17.0.0-alpha.4
  Release changelog notes for 16.20.0 (#4684)
  Update to latest backend versions (#4682)
  Support RelationList field with StaticCatalogVocabulary and SelectWidget. (#4614)
  Load a theme via a `theme` key in `volto.config.js` or in `package.json` (#4625)
  docs: improve creating view documentation (#4636)
  fix sitemap.xml.gz is not compressed #4622 (v2) (#4663)
  Make URL a literal string to fix broken link (#4667)
  Move developer guidelines to contributing #4665 (#4666)
  Update Volto contributing to align with and refer to the new Plone co… (#4634)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

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