-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
test: add shared procedures playground #6485
test: add shared procedures playground #6485
Conversation
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 was expecting to see some code relating to actually sharing procedures here, but as it is it seems to just create two workspaces. I suppose it can't be written yet because the APIs it depends on don't exist yet.
But it make me wonder: can this not be combined with the existing multi_playground.html
? Is there some reason we need multiple multi-Workspace
playgrounds? (And with four eight workspaces you can test more complex sharing behaviour, too…)
<script> | ||
var BLOCKLY_BOOTSTRAP_OPTIONS = { | ||
additionalScripts: [ | ||
'msg/messages.js', |
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.
Post #6475 this should be build/msg/en.js
—but in the mean time it could be build/msg/js/en.js
.
function start() { | ||
setBackgroundColour(); | ||
|
||
var toolbox = document.getElementById('toolbox'); |
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.
Any reason you're not using the new JSON toolbox format?
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.
Nope! Just missed this, will fix o7
I think it makes more sense to keep this separate and minimal for now. That way it's clear what code is important for sharing procedures. And I don't feel bad about being messy for testing heheh. But when I'm cleaning this up at the end of the quarter, I'd be happy to merge it with the existing multi playground code. |
<xml xmlns="https://developers.google.com/blockly/xml" id="toolbox" style="display: none"> | ||
<category name="Other Blocks" categorystyle="logic_category"> | ||
<block type="controls_if"></block> | ||
<block type="logic_operation"></block> | ||
<block type="logic_negate"></block> | ||
<block type="logic_boolean"></block> | ||
<block type="text_print"></block> | ||
</category> | ||
<category name="Variables" categorystyle="variable_category" custom="VARIABLE"></category> | ||
<category name="Functions" categorystyle="procedure_category" custom="PROCEDURE"></category> | ||
</xml> |
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.
Should this be deleted now?
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 approve of these changes even harder now.
Fixes #6512 |
The basics
npm run format
andnpm run lint
The details
Resolves
Blocked by #6475 cuz I need to change the
additionalScripts
to point to the built msgs.Proposed Changes
Adds a test page (which doesn't do anything yet) for eventually testing the shared procedures code.
Reason for Changes
Having a test page is going to be helpful for things that are more difficult to unit test.
Test Coverage
This is testing silly!
Documentation
Noup
Additional Information
N/A