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

Move block animation code to a new file and rebuild #1787

Merged

Conversation

rachel-fenichel
Copy link
Collaborator

@rachel-fenichel rachel-fenichel commented Apr 16, 2018

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

None

Proposed Changes

Move code for block animations to a new file.

Reason for Changes

Simplify merges with scratch-blocks. Generally make it easier for forks to override our animations without merge problems.

Test Coverage

Rebuilt, then checked that dispose, connection, and disconnect animations all play properly in the playground (uncompressed) and code demo (compressed).

Additional Information

See scratchfoundation/scratch-blocks#1449

Copy link
Contributor

@picklesrus picklesrus left a comment

Choose a reason for hiding this comment

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

I think this is a step in the right direction to support scratch-blocks use case, but I want to make sure we think about whether this makes animations harder in the future. e.g. if we introduce this class here, do we have to keep it and the resulting api (disconnectUiStop, disconnectUiEffect, connectionUiEffect etc.) forever? Looks like everything is now marked package or private so maybe not? Were all these methods package and private before? e.g. is Blockly games calling any of them such that we're going to have to change them to public :) Let me know if those thoughts aren't coherent... haven't quite hit the right balance of coffee/food/water post redeye :)

@rachel-fenichel
Copy link
Collaborator Author

I don't think this adds anything new, because everything is package or private.

Copy link
Contributor

@picklesrus picklesrus left a comment

Choose a reason for hiding this comment

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

LGTM

var svgGroup = block.getSvgRoot();
workspace.getAudioManager().play('delete');

var xy = workspace.getSvgXY(svgGroup);
Copy link
Contributor

Choose a reason for hiding this comment

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

how come you can drop the type annotation? I guess I don't quite know why it was needed before.

Copy link
Collaborator Author

@rachel-fenichel rachel-fenichel Apr 18, 2018

Choose a reason for hiding this comment

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

It was a cast before (from the perspective of closure-compiler). getSvgRoot is already annotated to return the correct type.

It might eventually need a cast inside it, but that's out of scope for right now. It gives warnings, not errors.

@rachel-fenichel rachel-fenichel merged commit 46da00d into google:develop Apr 18, 2018
@rachel-fenichel rachel-fenichel deleted the feature/block_animations branch April 18, 2018 17:17
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.

2 participants