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

Hooks to init search and detail data and manage search url params #301

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

BasLee
Copy link
Contributor

@BasLee BasLee commented Nov 28, 2024

Resolves #259, resolves #201, resolves #188, resolves #203

BasLee added 30 commits October 17, 2024 11:36
- Update URL first, state follows
- Deduplicate search param and query state
useEffect was triggered by isDirty, but the search state of useSearchUrlParams was not yet updated:
so the request was already sent but not aborted because useEffect did not include search state dependencies.
This commit also removes an almost identical useEffect that only ran when the searchTerms were present.
@BasLee BasLee changed the title Init detail hook Hooks to init search and detail data and manage search url params Nov 28, 2024
@svandaalen
Copy link
Collaborator

Er bevinden zich nog twee bugs rondom het detailscherm.

  1. Wanneer je een zoekresultaat aanklikt, zie je in de URL van het detailscherm ook nog alle search params, bv: http://localhost:5173/detail/urn:republic:inv-3800-date-1745-08-16-session-224-resolution-9?indexName=&fragmentSize=100&from=0&size=3&sortBy=_score&sortOrder=desc&query=eyJkYXRlRnJvbSI6IjE1NzYtMDgtMDQiLCJkYXRlVG8iOiIxNzk2LTAzLTAxIiwicmFuZ2VGcm9tIjoiMCIsInJhbmdlVG8iOiI2NjAwMCIsImZ1bGxUZXh0IjoicGFhcmQiLCJ0ZXJtcyI6e30sInJhbmdlRmFjZXQiOiJ0ZXh0LnRva2VuQ291bnQiLCJhZ2dzIjpbeyJmYWNldE5hbWUiOiJ0ZXh0VHlwZSIsIm9yZGVyIjoiY291bnREZXNjIiwic2l6ZSI6MTB9LHsiZmFjZXROYW1lIjoicmVzb2x1dGlvblR5cGUiLCJvcmRlciI6ImNvdW50RGVzYyIsInNpemUiOjEwfSx7ImZhY2V0TmFtZSI6InByb3Bvc2l0aW9uVHlwZSIsIm9yZGVyIjoiY291bn[...]&highlight=paard
  2. Wanneer je in het detailscherm op prev/next 'ding waarnaar je aan het kijken bent' (resoluties bij Republic, pagina's bij Globalise, brieven bij Suriano, etc.) klikt, dan verdwijnt de DetailSearchResultsNavigation.tsx component volledig. Ik vermoed dat het door deze if-check komt op LL37-39:
if (resultIndex === -1) {
    return null;
  }

Deze check const resultIndex = searchResults.results.findIndex((r) => r._id === tier2); levert -1 op wanneer je naar een annotatie gaat die niet in de zoekresultaten zit. Echter, het moet wel mogelijk zijn om naar annotaties te gaan die niet in de zoekresultaten zitten. Dit gaat namelijk wel de eerdergenoemde prev/next knoppen. Ik moet nog even nadenken over hoe we dit kunnen fixen.

@BasLee
Copy link
Contributor Author

BasLee commented Jan 6, 2025

Er bevinden zich nog twee bugs rondom het detailscherm.

  1. Wanneer je een zoekresultaat aanklikt, zie je in de URL van het detailscherm ook nog alle search params

Dit is geen bug maar een feature. (Zeg ik dit nu serieus?) Op deze manier kun je browsen in de zoekresultaten, ook als je refresht. Zie ook #259 en #259 (comment) Ook is deze state nu niet meer gedupliceerd in stores en de url, zie: #201

  1. Wanneer je in het detailscherm op prev/next 'ding waarnaar je aan het kijken bent' (resoluties bij Republic, pagina's bij Globalise, brieven bij Suriano, etc.) klikt, dan verdwijnt de DetailSearchResultsNavigation.tsx component volledig.

Dit is een lastige. En deze bug zit ook al gedeeltelijk in main (het component wordt in main nog wel getoond, maar de knoppen functioneren niet zoals zou moeten). Een oplossing zou zijn om de prev/next-search-result-knoppen wel te tonen maar in uitgeschakelde toestand (want we bekijken geen zoekresultaat meer), en de terug-knop te laten leiden naar de zoek-query zoals die in de url zit. Zie c180a72 voor deze oplossing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants