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
Instant visualization updates for artist #24196
Conversation
a9e0093
to
72514e6
Compare
Also added a "limited" version of autorun that
I'm not a huge fan of this mode, but we want to find a way to give you the convenience of autorun when fiddling with lengths and values, but still try to make you think about actual program flow when arranging blocks. |
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.
Can you explain a bit what problem the immovable blocks stuff is trying to solve?
= boolean_check_box f, :auto_run | ||
.field | ||
= f.label :limited_auto_run, 'Automatically rerun on block changes, but only allow field edits' | ||
= boolean_check_box f, :limited_auto_run |
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 instead be two potential modes for the single auto_run
field?
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.
Yup, done.
// Droplet doesn't automatically bubble up aceEditor changes | ||
this.studioApp_.editor.aceEditor.on('change', changeHandler); | ||
} | ||
this.studioApp_.addChangeHandler(this.rerunSetupCode.bind(this)); |
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.
👍
apps/src/turtle/artist.js
Outdated
if (!block.isMovable()) { | ||
this.immovableBlocks.push(block); | ||
} else { | ||
block.setMovable(false); |
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.
is this gonna mess up any blocks that are supposed to be immovable on initialization?
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.
It shouldn't because I'm tracking the blocks that were initially immovable in this.immovableBlocks
. However, it can cause problems if the project autosaves when you're in the immovable mode and you reload the page. I changed the implementation by adding lockMovement()
/unlockMovement()
methods to blockly in code-dot-org/blockly#145.
apps/src/turtle/artist.js
Outdated
@@ -341,6 +349,20 @@ Artist.prototype.init = function (config) { | |||
if (finishButton) { | |||
dom.addClickTouchEvent(finishButton, this.checkAnswer.bind(this)); | |||
} | |||
|
|||
if (this.autoRun) { |
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 any of this live in https://github.com/code-dot-org/artist? If a 3rd party wanted to create a demo like the one in the PR description .gif, what additional code would that require?
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.
That repo just has a visualizer, anyone who integrates it still has to provide their own code editor, and then decide when/how often to call display(). Just like I did in this PR, they would have to start calling display()
on every code change in their code editor, in addition to/instead of their version of a run button click.
I'd love to see a test coverage plan if this isn't something we're going to release in Curriculum right away. Basically: how do we get notified if some other change breaks this? Super awesome feature! |
When you merge, can you remove it from the Dessert Menu and send mail? Thanks! |
Curriculum team wants students to be able to figure out how a program would run before running it, but autorun would make it too easy to avoid practicing that skill on artist levels. However, they still want students to be able to take advantage of autorun to fiddle with angles and lengths, since that's not really what we're trying to teach, and it can be really annoying to wait for the whole program to run multiple times just to get one of those numbers right. |
Re: testing, I added better unit test coverage and a pretty basic eyes test. |
@Hamms PTAL |
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.
Love the testing!
bf805c2
to
1bcb675
Compare
Add a level option to immediately rerun code on block space changes.