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

fix: advanced playground and playground #6021

Merged
merged 4 commits into from
Mar 23, 2022

Conversation

alschmiedt
Copy link
Contributor

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

#6019 and the fact that the playground was not working when hosted.

Proposed Changes

  • Changes the prepare.js script to use absolute path instead of relative path. This means that it will work with both the playground and the advanced playground. This makes it possible to fix Hosted advanced playground times out #6019.
  • Moving to the absolute path also fixed a bug that made it impossible to load the scripts from hosted sites. (We were using ../.. instead of just ../)
  • Small fix for the test_theme that was not working.

Behavior Before Change

  • Playground and advanced playground did not work when hosted.

Behavior After Change

  • Advanced playground and playground now both work when hosted on appspot.
  • Regular playground works when hosted on google.github.io. We would still need to upload the dev-tools/index.js file to the gh-pages branch for this to work on google.github.io.

Reason for Changes

  • If we are going to keep the advanced playground in core then it might as well work.
  • Hosted playgrounds working is necessary for testing.

Test Coverage

  • Tested on google.github.io
  • Tested on appspot
  • Tested locally

Documentation

Additional Information

document.write('<script src="../../php_compressed.js"></script>');
document.write('<script src="../../python_compressed.js"></script>');
for (let i = 0; i < files.length; i++) {
document.write('<script src="/' + root + files[i] + '"></script>');
Copy link
Collaborator

Choose a reason for hiding this comment

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

For future: this seems like a good example of a place to use template strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So when writing this PR I remembered that we probably want at least the basic playground to work on ie11 to make testing easier. I don't think it currently will, but I didn't want to add another piece that we would have to go back and update.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohhh gotcha gotcha. Sweet! That makes sense

Comment on lines 23 to 28
goog.require('Blockly.libraryBlocks');
goog.require('Blockly.Dart.all');
goog.require('Blockly.JavaScript.all');
goog.require('Blockly.Lua.all');
goog.require('Blockly.PHP.all');
goog.require('Blockly.Python.all');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these should be bundled in by goog.bootstrap in prepare.js, could you check if these can be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I can remove those as well as the minimalist renderer and the zelos themes because it looks like those are getting exported in blockly.js.

tests/playgrounds/prepare.js Show resolved Hide resolved
Comment on lines +12 to 14
const TestHatsTheme = Blockly.Theme.defineTheme('testhats', {
'base': Blockly.Themes.Classic
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has side effects right? And that's how we can access the theme later? Just want to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this has side effects. We register themes in their constructors. However, I believe in this case the populateTestThemes method is how we are adding the themes to the advanced playground.

@BeksOmega BeksOmega removed the request for review from rachel-fenichel March 23, 2022 14:38
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.

3 participants