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

Size block #21322

Merged
merged 15 commits into from Mar 22, 2018
Merged

Size block #21322

merged 15 commits into from Mar 22, 2018

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Mar 19, 2018

This PR covers the addition of a new size block with a size input and sticker dropdown.
Previously, users did not have the option to input a custom size as shown in "initial option" below. The addition of the size block to the toolbox, gives users the option to input a custom size to enhance user experience.

Initial option
prework-stickersize

This is the implementation of the new size block that allows a user to input any numerical value for size. The sticker drawn is resized per the size input.

post-stickersizev2

Artist toolbox that includes two sticker options
sticker-rev1

@nkiruka nkiruka requested a review from joshlory March 19, 2018 16:56
@@ -1585,7 +1585,7 @@ Artist.prototype.step = function (command, values, options) {
this.visualization.ctxScratch.save();
this.visualization.ctxScratch.translate(this.visualization.x, this.visualization.y);
this.visualization.ctxScratch.rotate(this.visualization.degreesToRadians_(this.visualization.heading));
this.visualization.ctxScratch.drawImage(img, 0, 0, width, height, -width / 2, -height, width, height);
this.visualization.ctxScratch.drawImage(img, -width / 2, -height, width, height);
Copy link
Contributor

Choose a reason for hiding this comment

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

I repro the IndexSizeError: DOM Exception 1 with the following code:

phantomjs> img = new Image(100, 100);
phantomjs> ctx = document.createElement('canvas').getContext('2d');
phantomjs> ctx.drawImage(img, 0, 0, 100, 100);
IndexSizeError: DOM Exception 1

  phantomjs://repl-input:1 in global code

Can we continue to pass all 9 arguments here to fix the failing test?

@@ -1129,6 +1129,39 @@ exports.install = function (blockly, blockInstallOptions) {
'", null, \'block_id_' + this.id + '\');\n';
};

blockly.Blocks.turtle_sticker_with_size = blockly.Blocks.turtle_stamp = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why also export as turtle_stamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Redundant! Thanks for spotting.


dropdown = new blockly.FieldImageDropdown(values, 40, 40);

input.appendTitle(dropdown, 'VALUE');
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can share the code from line 1134-1149 and 1152-1155 with blockly.Blocks.sticker above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted "turtle_stamp" from lines 1132 & 1160.


generator.turtle_sticker_with_size = generator.turtle_stamp = function () {
var size = generator.valueToCode(this, 'SIZE', Blockly.JavaScript.ORDER_NONE);
return 'Turtle.drawSticker("' + this.getTitleValue('VALUE') + '", ' + size + ', \'block_id_' + this.id + '\');\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: this is a perfect use case for string interpolation 😄.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I concur!

@nkiruka
Copy link
Contributor Author

nkiruka commented Mar 21, 2018

Updated feedback. Please take a look.

@Hamms
Copy link
Contributor

Hamms commented Mar 21, 2018

"Set sticker size" implies to me that it's changing the size of the sticker drawn by the "draw sticker" block. I'd suggest we instead label this block something like draw sticker <sticker dropdown> at size <size input>

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

code LGTM, but I would like to see the block appearance changed before merging

this.setPreviousStatement(true);
this.setNextStatement(true);
this.setTooltip(msg.drawSticker());
this.setTooltip(msg.setStickerSize());
Copy link
Contributor

Choose a reason for hiding this comment

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

Does setting two tooltips make sense, or will the second override the first?

artist.step('sticker', ['Alien', size, blockId], options);

expect(setStickerSize).to.be.have.been.calledWith(img, 0, 0, 100, 40, -15, -12, 30, 12);

Copy link
Contributor

Choose a reason for hiding this comment

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

great tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@nkiruka
Copy link
Contributor Author

nkiruka commented Mar 22, 2018

Updated PR, please review.

@nkiruka nkiruka merged commit 2a50416 into staging Mar 22, 2018
@nkiruka nkiruka deleted the size-block branch March 22, 2018 16:21
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.

None yet

4 participants