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

[Google Blockly][Outbreak] Set up Outbreak level to work with Google Blockly #40613

Merged
merged 2 commits into from
May 18, 2021

Conversation

ajpal
Copy link
Contributor

@ajpal ajpal commented May 17, 2021

Most of the changes here are just adding stubs so that Outbreak, which is a Sprite Lab level, can load without error.

  • FunctionEditor - this is the modal function editor which is custom to our fork. Outbreak levels won't actually use this feature.
  • Block.createProcedureDefinitionBlock - Outbreak won't use procedure blocks
  • Variables getters and setters - we have custom logic to separate variables of different types. Outbreak levels won't use variables.
  • dispatchEvent - used in Sprite Lab when you switch from the costume tab to the code tab. Outbreak levels won't use the costume tab.
  • New constants: DEFINITION_BLOCK_TYPES and BlockValueType
  • prefixLines - Our code expects this function to be defined on Blockly.Generator but in Google Blockly it's on Blockly.JavaScript, so just aliasing it solves the problem.

There's a couple small pure refactor changes in here as well:

  • Pulling out some constants from googleBlocklyWrapper to cdoConstants
  • Renaming initializeBlocklyXml to initializeTouch in CdoTouch

image

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@ajpal ajpal requested a review from a team May 17, 2021 17:09
// TODO
openEditorForFunction(procedureBlock, functionName) {}

// TODO
Copy link

Choose a reason for hiding this comment

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

Not sure what works for your workflow, but do you have a task or reminder for at the 'end' of getting Outbreak onto Blockly to go back and make sure that all the 'ToDos' are addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not all of these TODOs need to be addressed before HOC. In fact, I think most of them will not be addressed as part of Outbreak work. The stubs will need to be filled in when we migrate all of Sprite Lab, but leaving them as no-op stubs for now is fine because Outbreak doesn't actually use the features

Copy link

@epeach epeach May 17, 2021

Choose a reason for hiding this comment

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

I would argue that we should add additional details to the ToDos then - to indicate the lifespan on the ToDo and differentiate between ToDos that require action and expected for-the-time-being No-Op functions .

@epeach
Copy link

epeach commented May 17, 2021

Is there a technical doc that outlines our approach/philosophy/process to moving things onto Blockly? It might be useful to have just one or two pages that outlines the process and major steps.

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.

This looks good to me.

@ajpal
Copy link
Contributor Author

ajpal commented May 17, 2021

Is there a technical doc that outlines our approach/philosophy/process to moving things onto Blockly?

These are probably a good place to start:
https://docs.google.com/document/d/1UG-TSEfdMglzOmZbvWeyNXtUm3YfEborpRIWHgHlBjA/edit
https://docs.google.com/document/d/1kVVnTRfmQQur62uVca4MKIu8SHHA6bWHSIFvfyzOcfE/edit#

@epeach
Copy link

epeach commented May 17, 2021

Immediately after leaving that comment, I read the Blockly Status doc and was like, "Oh, well, there it is!". 😆

@ajpal ajpal merged commit 3dd76a8 into staging May 18, 2021
@ajpal ajpal deleted the may14-outbreak-blockly branch May 18, 2021 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants