-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement pagination on Elements page #189
base: dev
Are you sure you want to change the base?
feat: implement pagination on Elements page #189
Conversation
…rsor handling - Updated cache logic to replace the current search page instead of merging results. - Introduced `searchKey` and `pageKey` functions for better cache key management. - Removed redundant `SEARCH_CATALOG_MORE_QUERY` and adjusted related logic in `ElementsPage`. - Implemented pagination controls in `ElementsPage` for navigating between pages. - Added utility functions for managing scroll positions during result loading. - Enhanced tests to cover new pagination behavior and ensure correct cache interactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude review:
- Resultatlisten forsvinner mens neste side lastes. Med 700 ms treg API ble listen byttet ut med «Loading elements…», og pagineringen forsvant. På dev ble de gamle radene stående (previousData). Det er en synlig UX-forverring og gir layout-hopp. Dette er det eneste jeg ville rettet før merge.
- Forvirrende tekst under listen. Den sier «Page 29, showing 2 results», «Showing 2 of 562» og «Page 29» samtidig. «Showing 2 of 562» betydde før «alle hittil», men betyr nå bare denne siden. Det er misvisende og duplisert.
- hvorfor telles totalcount på hver side? er det ikke alltid maks med unntak av hvis det er siste side? totalCount telles på hver side. Det er en bevisst avgjørelse som er dokumentert i docs/API.md: kostnaden er cappet, ca. 30–100 ms. Men det går på bekostning av bærekraft-poenget i issuet.
- Hvorfor har du disablet eslint for react hooks? fra pkt: Små ting. cache.gc() kjører etter hver datalasting, og det er en eslint-disable for react-hooks..
- Uenig i valg av url, den er litt vel lang. URL-en vokser per side. Hver cursor er ca. 100 tegn. På side 29 var adressen 3067 tegn. Det er dokumentert og greit under nettlesergrensene, men vær klar over det. Claude foreslår: Jeg anbefaler å lagre stakken i location.state. Det er en liten endring i ElementsPage.tsx, og URL-en slutter å vokse. Prisen er at en delt lenke til side 5 mister Previous-historikken. Det som gjenstår som ulempe, er at det blir to kilder til tilstand (URL og state) som må holdes i synk. Det må testes, blant annet reload, Tilbake/Frem og en lenke uten state.
- Update pagination logic to use cursor-based navigation with `after` parameter instead of `cursor`. - Modify displayed result text to show ranges (e.g., "Showing 1–20 of 562") instead of total counts. - Adjust GraphQL queries to conditionally request total counts based on pagination state. - Enhance test cases to validate new pagination behavior and result display. - Ensure that the page state is preserved across navigations and reloads, improving user experience.
|
Anbefaler å legge inn at den går til første side ved feil i lenken (eks utdatert cursor) for brukeropplevelsen. nå går den vel til try again eller en feil. fokus etter feil er noe vi burde tenke på. pageChangePendingFocus blir stående true hvis hentingen feiler. Fokus hopper da til «Results» ved en senere, urelatert lasting. du har fortsatt eslint disable på en react hook, men kommer ikke til å blokkere merge på det. så resolve conflicts og så burde det være good to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fiks det jeg kommenterte på, så kan du merge den direkte selv :). Får ikke sett på det på en stund så da er nok dette beste løsning.
searchKeyandpageKeyfunctions for better cache key management.SEARCH_CATALOG_MORE_QUERYand adjusted related logic inElementsPage.ElementsPagefor navigating between pages.