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

Jigsaw UX Changes #20000

Merged
merged 3 commits into from Jan 17, 2018
Merged

Jigsaw UX Changes #20000

merged 3 commits into from Jan 17, 2018

Conversation

ryansloan
Copy link
Contributor

@ryansloan ryansloan commented Jan 13, 2018

Responding to frequent user feedback and classroom observations, making a couple small tweaks to Jigsaw puzzles:

  • removing the pink "loop-like" block from the levels - students don't learn loops until later, and we never actually explain how to use this block in Jigsaw.

  • Numbering the color-matching blocks These are hard to match to the partially transparent preview on bad screens, and could also present problems for students with color acuity issues or colorblindness

Numbered Jigsaw blocks

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

This is so awesome! 🎉

blockly.Blocks[name] = {
helpUrl: '',
init: function () {
this.setHSV.apply(this, hsv);
this.appendDummyInput()
.appendTitle(new blockly.FieldImage(skin.blank, width, 54));
.appendTitle(new blockly.FieldImage(skin.blank, (width-8)/2, 54))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change of width -> (width-8)/2?

Tiny nit: calculate this once above, and store it in a descriptive variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, width was used to supply the total width of the block, but this is only accounting half of it, but this function is only used here, so I can switch to just passing in a smaller value for width if that would be clearer

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think it's a little cleaner to update the places where we call generateBlankBlock above with the new value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or just inline it as always 36 and remove the parameter, now that it's always the same value!

.appendTitle(new blockly.FieldImage(skin.blank, width, 54));
.appendTitle(new blockly.FieldImage(skin.blank, (width-8)/2, 54))
.appendTitle(new blockly.FieldLabel(label,
{fixedSize: {width: (width-8)/2, height: 64}, fontSize:32}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: fontSize: 32 (space after the colon).

.appendTitle(new blockly.FieldImage(skin.blank, (width-8)/2, 54))
.appendTitle(new blockly.FieldLabel(label,
{fixedSize: {width: (width-8)/2, height: 64}, fontSize:32}))
.setAlign(Blockly.ALIGN_CENTRE);
this.setPreviousStatement(true);
if (hasAppend) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this can go away now 🎉

Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

LGTM!

@ryansloan ryansloan merged commit de82f27 into staging Jan 17, 2018
@jeremydstone
Copy link

Congrats on your first PR @ryansloan!

@breville
Copy link
Member

Great work, mysterious contributor @ryansloan.

@balderdash balderdash deleted the jigsaw-ux-changes branch September 20, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants