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

test: add shared procedures playground #6485

Merged
merged 3 commits into from
Oct 13, 2022

Conversation

BeksOmega
Copy link
Collaborator

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm 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

@BeksOmega BeksOmega requested a review from a team as a code owner October 5, 2022 16:31
@BeksOmega BeksOmega requested a review from cpcallen October 5, 2022 16:31
Copy link
Contributor

@cpcallen cpcallen left a 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',
Copy link
Contributor

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');
Copy link
Contributor

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?

Copy link
Collaborator Author

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

@BeksOmega
Copy link
Collaborator Author

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…)

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.

Comment on lines 180 to 190
<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>
Copy link
Contributor

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?

Copy link
Contributor

@cpcallen cpcallen left a 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.

@BeksOmega BeksOmega merged commit ad22de1 into google:develop Oct 13, 2022
@BeksOmega
Copy link
Collaborator Author

Fixes #6512

@BeksOmega BeksOmega deleted the test/shared-procedures-page branch May 14, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants