-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added Mirador redux store observer + keep track of current Mirador canvas in Zustand store #330
base: main
Are you sure you want to change the base?
Conversation
…nvas in Zustand store
src/components/Mirador/Mirador.tsx
Outdated
@@ -82,6 +90,8 @@ export function Mirador() { | |||
} | |||
|
|||
function performPostInitialisationActions() { | |||
observeMiradorStore(viewer.store, projectConfig.id, onCanvasChange); |
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.
Moet er nog iets met de 'unsubscribe' gebeuren?
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.
Goede! Ik heb het toegevoegd in 63d52c3.
if (projectConfig.visualizeAnnosMirador) { | ||
const newCanvas = miradorStore?.getState().windows[projectName] |
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.
Kun je hier ook de mirador canvas slice gebruiken?
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.
Ik heb het geprobeerd, maar op dit punt van de code loopt currentCanvas 1 achter op newCanvas. Bv. je zit op 131r en gaat naar 131v, dan is newCanvas netjes 131v, maar currentCanvas is hier dan nog 131r. De slice wordt dus niet snel genoeg geupdate, terwijl de interne Mirador state wel tijdig geupdate is.
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.
A, volgens mij snap ik hem, omdat je hier de change dispatcht in de internal store, is de canvas alleen in de internal store up to date?
Als de internal store leidend is, misschien dan simpeler om een util functie te maken van miradorStore?.getState().windows[projectName].canvasId as string;
.
En dan kan de canvas slice weg, aangezien die enkel maar kijkt naar de internal store?
if (projectConfig.visualizeAnnosMirador) { | ||
const newCanvas = miradorStore?.getState().windows[projectName] |
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.
En hier?
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.
Zie boven.
…useEffect so the observer gets properly subscribed and unsubscribed when the component gets reloaded
Fixes #178.
This PR adds:
Using the current canvas state from the Zustand store in the
BrowseScanButtons
component (combined with the new observer) fixes the link issue between the prev/next scan buttons and the page break markers. Previously, when the user clicked on one of the page markers, the prev/next scan buttons still thought the user was on the first scan. Now, the (state behind the) prev/next scan buttons update correctly when the user clicks one of the page markers.