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

Extract components for handling OSD + tile sources. #4014

Merged
merged 1 commit into from
Dec 9, 2024
Merged

Conversation

cbeer
Copy link
Collaborator

@cbeer cbeer commented Dec 4, 2024

This PR extracts components from OpenSeadragonViewer that manage the integration between OSD + react. By expressing the tile source as a react component, we're able to take advantage of react's internal rendering smarts instead of having to managing what may have changed.

@cbeer cbeer force-pushed the osd-fn branch 9 times, most recently from a2c3624 to 58560de Compare December 5, 2024 22:06
@cbeer cbeer marked this pull request as ready for review December 5, 2024 22:28
@marlo-longley
Copy link
Member

marlo-longley commented Dec 6, 2024

This looks great. Thank you Chris! Small comments --

  1. needs a rebase

  2. my main question which could be due to lack of understanding is -- if we can reduce the number of references to viewer and rely more on your new OpenSeadragonViewerContext. In my reading we have:

  • setViewer in the parent component's state that gets passed down
  • apiRef in the parent which is also holding a viewer and uses OSD methods like fitBounds
  • the viewerRef in the child OpenSeadragonComponent
  • also the viewer held in OpenSeadragonViewerContext

Something to think about (this also just helped me understand to write it out) -- and maybe for down the road not adding more work to this PR.

@cbeer
Copy link
Collaborator Author

cbeer commented Dec 6, 2024

I think your analysis is good. I don't understand the difference between apiRef + viewer in the component state and why we may or may not need both (I think we're using it as a hack to force a re-render when we initialize the viewer?), and I think the viewer prop could be replaced with a context consumer eventually (although we send it through to plugins which may need to be updated too).

@marlo-longley marlo-longley merged commit acccdda into master Dec 9, 2024
7 of 8 checks passed
@marlo-longley marlo-longley deleted the osd-fn branch December 9, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants