Closed frasercl closed 3 months ago
Could you clarify who you wanted to review this PR? (I'm planning to because I'll be working off of these changes, but I saw there are four(!) people listed as requested 😆)
Could you clarify who you wanted to review this PR? (I'm planning to because I'll be working off of these changes, but I saw there are four(!) people listed as requested 😆)
No worries, I haven't started the review and can opt out!
Review time: medium-large
Refactors state representation out of
App
in favor of a new component,ViewerStateProvider
.Some background. This repo is, in a sense, concerned primarily with providing a nice UI for manipulating viewer state. By viewer state, I mean any state which is in some sense a setting of volume-viewer - so whether the bounding box is visible is viewer state, but whether the control panel is open is not. Currently, this state is stored and managed like this:
App
.App
's other responsibilities - managing image loading, updating the actual viewer, keeping track of basic UI state - we define several setter functions which keep us a bit disciplined about updating viewer state.App
, which is a problem for upcoming work on populating URLs with viewer state for sharing.App
. WhileApp
has props for setting viewer state, they only set initial state and do nothing when the component is mounted.The changes in this PR move us to managing state like this:
ViewerStateProvider
.App
, allowing access to viewer state outside the viewer component.connectToViewerState
, to "subscribe" to the props they care about directly, cutting down on prop drilling.