Monitor save load #3713
Monitor save load #3713
Conversation
kchadha
force-pushed
the
monitor-save-load
branch
from
a823b9b to
5c50924
Compare
|
？？？？？？？ |
|
There is a problem, if the previous Project has variables, then load a new project(directly use loadproject function in vm with json file) ,these variables will appear in the current project(need to switch sprites and then variables will appear in variable catalog). it may caused by monitorBlocks in runtime.js in scratch-vm. if clean the monitorBlocks in dipose function in runtime.js will solve this problem. |
paulkaplan
left a comment
paulkaplan
left a comment
There was a problem hiding this comment.
Changes look good, but you can resolve the TODO comment about removing state.mode as mentioned in the review comment.
src/containers/monitor.jsx
Outdated
| const modeIndex = modes.indexOf(this.state.mode); | ||
| this.setState({mode: modes[(modeIndex + 1) % modes.length]}); | ||
| const newMode = modes[(modeIndex + 1) % modes.length]; | ||
| this.setState({mode: newMode}); |
There was a problem hiding this comment.
You can remove all of the references to "mode" stored in state and just consume it from props, since we are now updating it through the VM. So you can remove lines 36-39, 106, 113, 120, 127, and update lines 100 and 143 to use this.props.mode.
|
@summerscar, thank you for calling out that issue! Since this is an issue that already exists, I filed it as a bug in scratch-vm to be fixed separate from this pull request. |
…positioned or dragged, a new location should be recorded in the monitor data.
kchadha
force-pushed
the
monitor-save-load
branch
from
50b8105 to
4ec54e3
Compare
5 participants
Resolves
Proposed Changes
Add ability to save and load monitors in 3.0 projects.
Monitors retain position and mode, and in the case of list monitors, height and width across save and load.
Make extension monitors the right color, and make the extension color the default color for monitors when a color is not specified.
Test Coverage
Manual testing.
Test instructions in scratchfoundation/scratch-vm#1686.
Testing branch: https://llk.github.io/scratch-gui/monitors/
Related PRs
This PR is dependent on scratchfoundation/scratch-vm#1686. The VM PR should be merged first.
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet
cc/ @picklesrus