-
Notifications
You must be signed in to change notification settings - Fork 519
Sketch Lab: reset across level changes #68685
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
base: staging
Are you sure you want to change the base?
Conversation
}; | ||
}, []); | ||
|
||
useEffect(() => { |
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.
Does this useEffect
only fire on level change or does it fire every time sources change?
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.
It fires when sources change (which is debounced to max every 500ms), but only actually remounts when the level changes (or a teacher switches between students). I chatted with Sanchit and modeled it after something similar we're doing in DanceView:
code-dot-org/apps/src/dance/lab2/views/DanceView.tsx
Lines 323 to 334 in b11ab65
useEffect(() => { | |
if (!workspace.current) { | |
return; | |
} | |
const blocks = Blockly.serialization.workspaces.save(workspace.current); | |
if (!isEqual(blocks, currentSources.source)) { | |
loadBlocksToWorkspace( | |
workspace.current, | |
JSON.stringify(currentSources.source) | |
); | |
} | |
}, [currentSources.source]); |
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.
Mind adding a comment explaining that it only remounts if the level changes?
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.
Yep can do!
This PR adds support for resetting the canvas when a change to the current sources are observed (eg, when navigating between levels).
An earlier version of this PR used a more targeted approach of updating only specific bits of state on the existing Excalidraw canvas, vs. a full remount of the canvas (via a
key
prop). I was torn on which was preferable, but landed on a full remount (for now). There were a couple of problems with the more targeted approach:files
property (ie, images) that we serialize and deserialize from S3. The Excalidraw API appears to offer mechanisms toaddFiles
and togetFiles
, but no explicit "I want to set the files available to something else" (ie,setFiles
)?updateScene
) accepts the "elements" of your sketch (which I needed to deep clone to get to work, I am not sure why) and "app state", but not files.Handily, thanks to the
SourcesContainer
component, this appears to also give support for updating the Excalidraw canvas when a teacher switches between students, not just when switching between levels as a single user.Links
Testing story
I tested manually navigating between levels as an individual user, as well as navigating between students as a teacher.