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

Fixed toolbox overlay issue in spritelab. #28384

Merged
merged 1 commit into from May 10, 2019
Merged

Conversation

jmkulwik
Copy link
Contributor

@jmkulwik jmkulwik commented May 6, 2019

Spritelab top instructions had a bug where the instruction space would resize when switching between code and animation mode. This bug had always existed, but was not exercised until animation mode was added. The issue was not visibly recognized because resize events were being called extra times. In project mode, however, the resize events were causing rendering problems with the toolbox. This PR removes the extra resize events and resizes the instruction space only when needed.

Note: The red text difference in the 'instruction resize' gifs is due to a known issue in staging. Not due to this change.

Manual sanity checks performed:

  • dancelab is unchanged
  • gamelab is unchanged
  • hints still render as expected
  • expand/collapse still works as expected

OLD instruction resize:
shrinkingInstructionsOld

NEW instruction resize:
shrinkingInstructionsNew

OLD toolbox rendering:
image

NEW toolbox rendering:
image

@jmkulwik jmkulwik requested review from ajpal, epeach and joshlory May 6, 2019 20:30
@epeach
Copy link

epeach commented May 8, 2019

This looks similar to the change that was reverted which caused the toolbox to not render. Do you know what caused that regression and how this PR won't protects against that?

@jmkulwik
Copy link
Contributor Author

jmkulwik commented May 9, 2019

This looks similar to the change that was reverted which caused the toolbox to not render. Do you know what caused that regression and how this PR won't protects against that?

Yes. The previous PR which ended up causing a regression (#28294) was caused by a circular dependency.

Window.resize, which was called by Gamelab/actions.changeInterfaceMode was what caused the blockly blocks to appear under the toolbox. In a level that was part of a progression, however, changing to design mode also caused the instruction section of the screen to resize which in turn caused window.resize to be called again. My other version caused a break because when the instruction section resizes off-screen in design mode, the blockly blocks disappear. When switching back to code mode, the window.resize in actions.changeInterfaceMode was required in order to make the blocks show up again.

In both cases, however, we are calling window.resize far too often and are changing the size of elements on the screen when we don't need to. This new version removes the extraneous calls to window.resize and, as a side-effect, fixes a long-standing bug where the instruction window would resize when design mode rendered over it. Now, the instruction window resizes only when we want it to rather than at a response to a blockly resize command.

@epeach
Copy link

epeach commented May 9, 2019

^ Awesome simplification! Thanks for the explanation.

Copy link

@epeach epeach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jmkulwik jmkulwik merged commit 03132e5 into staging May 10, 2019
jmkulwik added a commit that referenced this pull request May 10, 2019
wjordan added a commit that referenced this pull request May 10, 2019
Revert "Merge pull request #28384 from code-dot-org/fix-resize-events"
@jmkulwik jmkulwik deleted the fix-resize-events branch August 9, 2019 23:25
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.

None yet

3 participants