Disallow instructions resize #30196
Disallow instructions resize #30196
Conversation
…isallow-instructions-resize
| @@ -45,6 +46,9 @@ export function changeInterfaceMode(interfaceMode, spritelabDraw) { | |||
| dispatch(show(Goal.NEW_ANIMATION)); | |||
| dispatch(pickNewAnimation()); | |||
| } | |||
| dispatch( | |||
| setAllowInstructionsResize(interfaceMode === GameLabInterfaceMode.CODE) | |||
| ); | |||
There was a problem hiding this comment.
Should we just allow redux/instructions.js to respond to the CHANGE_INTERFACE_MODE action?
Pros: We push some more business logic down into redux. We don't introduce a new redux action that might not make sense to dispatch in isolation.
Cons: We entangle two redux modules? But not much, because the CHANGE_INTERFACE_MODE action is defined in this file, I think.
Con:
There was a problem hiding this comment.
I like where you're going with this, but I don't think we want to do exactly what you're suggesting because it doesn't make sense for the instructions redux to know which modes it should be visible in. For example, instructions are visible in applab when not in code mode.
If we wanted to improve our separation of concerns, one idea is that the GameLabView could set the isVisible prop on InstructionsWithWorkspace. InstructionsWithWorkspace would then use that information to set the allowResize prop on TopInstructions (and possibly also on CodeWorkspaceContainer if we someday want to make it stop triggering resize events).
one nuance is that I'm suggesting allowResize over hidden or visible in part because TopInstructions already has a hidden prop which means something different -- i.e. set height to zero, as opposed to temporarily disallow changing the height.
Does this seem worth doing now?
There was a problem hiding this comment.
I see what you're saying. I think it's fine to use your current solution and unblock starlabs. Thanks for talking through alternatives!
There was a problem hiding this comment.
thinking on this a moment longer, there are 3 steps that would take us from the solution in this PR to the one I'm describing in the above comments:
- move responsibility for communicating with instructions from p5lab redux to GameLabView
- move understanding that "instructions are not visible" means "do not allow instructions resize" out of p5lab and into instructions
- change allowResize from redux to an explicitly passed prop
if we want to move in this direction, let's be deliberate about which of the above are important. if we only care about (2), for example, then we might only need to rename setAllowInstructionsResize to setIsInstructionsVisible.
There was a problem hiding this comment.
sorry I missed your comment from a couple minutes ago. ok, merging as-is sounds good!
Codecov Report
@@ Coverage Diff @@
## staging #30196 +/- ##
==========================================
Coverage ? 71.94%
==========================================
Files ? 1368
Lines ? 84496
Branches ? 3408
==========================================
Hits ? 60788
Misses ? 20463
Partials ? 3245
Continue to review full report at Codecov.
|
finishes https://codedotorg.atlassian.net/browse/LP-533
Background
although the problem pictured in the bug report no longer repros exactly, a slight rendering glitch still exists when switching between spritelab Code and Costumes modes:
The problematic sequence of events is:
code-dot-org/apps/src/p5lab/GameLabView.jsx
Lines 67 to 68 in 19d0eca
code-dot-org/apps/src/templates/CodeWorkspaceContainer.jsx
Lines 34 to 36 in 5702171
EXPECTED: blockly and instructions appear exactly as they were before
ACTUAL: instructions are the wrong size, and blockly workspace initially fails to appear
the above problem is exacerbated when we try to remove extraneous resize events from the tab-switching sequence, which can be seen in the screenshots in #28361 .
Description
This PR allows the application to specify whether the instructions pane is allowed to resize. SpriteLab and GameLab then disallow resizing while not in Code mode. This solves the problem and reliably gives the expected behavior listed above, even when stacked on top of #28294 (which is the one that led to the broken screenshots shown in #28361).