-
Notifications
You must be signed in to change notification settings - Fork 523
Remove CDO Blockly Wrapper and stop importing code-dot-org/blockly #70175
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
Conversation
This reverts commit bfd59c2.
StudioApp.loadBlocks is no longer additive
🖼️ Storybook Visual Comparison Report✅ No Storybook eyes differences detected! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file became redundant with the modifications to setupBlocklyGlobal made here: https://github.com/code-dot-org/code-dot-org/pull/66717/files#diff-01014c0cae568cca0fb3270c483ad8f697a04895cbaedcc4c1e4623a50654a9a
molly-moen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🥳 🎉
| app: app_options[:app], | ||
| use_droplet: use_droplet, | ||
| use_google_blockly: use_google_blockly, | ||
| use_blockly: use_blockly, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a stupid tangentially related question: use_blockly now references this statement, yeah?
use_blockly = !use_droplet && !use_netsim && !use_weblab && !use_javalab
We don't need to maintain that conditional (for Music Lab, Web Lab 2, etc.) because all of our new labs are being added via Lab2, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't touch use_blockly but that sounds plausible to me, but I'd have to confirm. Also Music Lab doesn't use any of the tools listed in that conditional, so I'd expect it to evaluate to true in any case.
ebeastlake
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possibly silly question (and maybe we'd want to rename that param in the future, to something like uses_nonlab2_blockly for clarity, depending on the answer), but otherwise, the PR LGTM and what an exciting milestone!
Last year, we completed the migration of all student/teacher facing labs to mainline Blockly. However, we were still maintaining both Blockly wrappers due to:
?blocklyVersion=cdointernal, primarily for gut-checking and regressionsThe latter issue has been resolved with:
As far as I am aware, we are no longer relying on n the URL parameter, so it feels like the right time to finally take the next big step. This work does the following:
blocklyVersionfrom controller and disables the URL paramblockly.jsscript, which would initialize the cdo Blockly WrappercdoBlocklyWrapper.jsand directly related util filescode-dot-org/blockly(andpepjs)Note that
pepjswas used to make Sprite Lab's location picker field work with CDO Blockly. [Slack context from 2021]Testing story
No test changes are expected here as this whole branch is essentially cleaning up code that is no longer used!
Follow-up work
Rename and being unwinding the Google Blockly Wrapper.
PR Creation Checklist: