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

Render Piskel inside Gamelab via npm #8780

Merged
merged 3 commits into from Jun 7, 2016
Merged

Render Piskel inside Gamelab via npm #8780

merged 3 commits into from Jun 7, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Jun 6, 2016

Makes the new @code-dot-org/piskel npm package an apps dependency, copies its contents to the built apps package (via a piskel-root utility so we don't depend on an exact location with node_modules), and renders it within an iframe on the animation tab. No other interaction yet.

screenshot from 2016-06-06 14-28-49

This is a new approach to a problem I originally tried to tackle in #8562.

Makes the new [@code-dot-org/piskel](https://www.npmjs.com/package/@code-dot-org/piskel) npm package an apps dependency, copies its contents to the built apps package, and renders it within an iframe on the animation tab.  No other interaction yet.
@@ -63,6 +64,7 @@ module.exports = function (grunt) {

var ace_suffix = envOptions.dev ? '' : '-min';
var dotMinIfNotDev = envOptions.dev ? '' : '.min';
var piskelRoot = String(child_process.execSync('`npm bin`/piskel-root')).replace(/\s+$/g,'');

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 6, 2016

Author Member

The newline in the output of this utility caused issues within grunt-contrib-copy; I'll simplify this as soon as I fix up the utility.

@@ -125,6 +127,12 @@ module.exports = function (grunt) {
},
{
expand: true,
cwd: piskelRoot,
src: '**',
dest: 'build/package/js/piskel/'

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 6, 2016

Author Member

I'll need to replace this copy step with the webpack equivalent once @pcardune's refactor goes in.

This comment has been minimized.

Copy link
@pcardune

pcardune Jun 6, 2016

Contributor

the webpack changes don't touch any of the copy logic. so this will just work.

This comment has been minimized.

Copy link
@pcardune

pcardune Jun 7, 2016

Contributor

do the files in here need to be minified? Or are they already minified?

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

They are already minified via Piskel's build process.

As-we-type I am working out a nice way to run debug Piskel inside gamelab (it's not as easy as doing a debug build) but that shouldn't block this change.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Jun 7, 2016

Assigning now that webpack is merged!

TODO: Piskel editor goes here!
</div>
</div>
<iframe id="piskel-frame" style={styles.editorColumn} src="/assets/js/piskel/index.html" />

This comment has been minimized.

Copy link
@islemaster

islemaster Jun 7, 2016

Author Member

Now that I think about it, this path might not work in environments where we fingerprint assets. I'll revisit this.

@pcardune

This comment has been minimized.

Copy link
Contributor

pcardune commented Jun 7, 2016

LGTM

We were getting Piskel via /assets which is fingerprinted via the Rails Asset Pipeline in certain environments.  Now we get it via /blockly (which should really be renamed to /apps) where it's served as simple static assets.
@islemaster islemaster merged commit 2230104 into staging Jun 7, 2016
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!
@islemaster islemaster deleted the npm-piskel branch Jun 7, 2016
deploy-code-org added a commit that referenced this pull request Jun 7, 2016
2230104 Merge pull request #8780 from code-dot-org/npm-piskel (Brad Buchanan)
69e5430 Merge branch 'test' into staging (Josh Lory)
d9c4601 Merge branch 'production' into test (Josh Lory)
a895055 Merge pull request #8811 from code-dot-org/test (Josh Lory)
0585f54 Merge pull request #8783 from code-dot-org/pcardune-content-security-policy (Paul Carduner)
e873ec7 Merge pull request #8786 from code-dot-org/pcardune-apps-coverage-report (Paul Carduner)
2d57ffd Merge pull request #8809 from code-dot-org/revert-8748-firebase-basics (David Bailey)
0e85abc Revert "Firebase basics" (David Bailey)
bdf3fbb Merge pull request #8784 from code-dot-org/hide_solution_link_for_pd (Mehal Shah)
c274d51 Merge pull request #8808 from code-dot-org/hotfix-progress-dots (Josh Lory)
3fda3ff Hotfix: fix unplugged progress dots rendering issue (Josh Lory)
e1bcd99 Amend 799c13c (Josh Lory)
bfde939 Merge pull request #8787 from code-dot-org/bounce-ui-tests (David Bailey)
460c2d6 Merge pull request #8748 from code-dot-org/firebase-basics (David Bailey)
85bfdce Merge pull request #8794 from code-dot-org/fixPrivilegesUI (ashercodeorg)
fa1d792 Merge pull request #8788 from code-dot-org/fix-test-iphone-hocReset (Andrew Oberhardt)
0ae0e16 Merge pull request #8791 from code-dot-org/cucumber-logs-on-s3 (Brad Buchanan)
d36fa89 Merge pull request #8806 from code-dot-org/test (Josh Lory)
a644205 PR feedback - going back to using forbidden (Mehal Shah)
fc95f09 Merge pull request #8804 from code-dot-org/staging (Josh Lory)
c373dff Merge pull request #8803 from code-dot-org/progress-dots-fix (Josh Lory)
799c13c Unplugged dots should be small when not the current level (Josh Lory)
9baccd6 Merge pull request #8801 from code-dot-org/localsLevel (Brent Van Minnen)
6826eeb locals.level instead of just level (Brent Van Minnen)
5d82aee Merge remote-tracking branch 'origin/staging' into cucumber-logs-on-s3 [ui test] (Brad Buchanan)
267321f Extract log uploader from runner.rb (Brad Buchanan)
5a59be7 Merge pull request #8800 from code-dot-org/staging (Brent Van Minnen)
436eee6 Use branchname as S3 prefix for log (Brad Buchanan)
ce5fd7a Merge pull request #8797 from code-dot-org/pcardune-fix-netsim (Brent Van Minnen)
d48a4ad Use locals to refer to variables passed in (Paul Carduner)
1341eda Merge pull request #8756 from code-dot-org/react-progress-inline-styles (Josh Lory)
f28869e Load Piskel via nonfingerprinted path (Brad Buchanan)
6d6e7d0 Code review feedback [ci skip] (Josh Lory)
76ad952 Merge pull request #8796 from code-dot-org/levelbuilder (Josh Lory)
e53f5c2 Level builder changes (Continuous Integration)
5e53305 Add some more documentation (Paul Carduner)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.