-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Block min width; notch offsets #2537
Block min width; notch offsets #2537
Conversation
@@ -316,13 +316,16 @@ Blockly.BlockRendering.RenderInfo.prototype.getInRowSpacing_ = function(prev, ne | |||
if (prev.isField() && prev.isEditable) { | |||
return BRC.MEDIUM_PADDING; | |||
} | |||
// A block with just an icon is skinny. Make it more substantial. |
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.
What if it's two or three icons? Is this adding necessary padding after the last icon or just bumping up the minimum block length? If the latter, should the block minimum width be bigger even for empty 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.
This is adding padding after the last icon no matter what. I think that's the right thing to do, because then a block with lots of icons still noticeably looks like a block instead of a blob.
Here's (some of) the code that controls this from the old rendering:
inputRows.rightEdge = iconWidth + Blockly.BlockSvg.SEP_SPACE_X * 2;
if (this.previousConnection || this.nextConnection) {
inputRows.rightEdge = Math.max(inputRows.rightEdge,
Blockly.BlockSvg.NOTCH_WIDTH + Blockly.BlockSvg.SEP_SPACE_X);
}
Min width for empty blocks: Empty blocks look like this right now:
which matches the old rendering and (imo) has enough width to show that it's a block and not a sliver.
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.
sgtm. Just update the comment, then.
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.
Done.
@@ -389,8 +392,10 @@ Blockly.BlockRendering.RenderInfo.prototype.getInRowSpacing_ = function(prev, ne | |||
|
|||
// Spacing between a rounded corner and a previous or next connection | |||
if (prev.isRoundedCorner()){ |
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.
Would it make sense to change the squared corners so they're the same size as a rounded corner? It seems odd to have special spacing for one and not the other.
Also, this block is splitting two checks for prev.isSquareCorner() that should be combined.
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.
I just tried making squared corners have width (previously they had no width, and all width was taken up in the spacer instead).
The problem is that a square corner can come before a hat (in which case the corner should have zero width) or before a notch (in which case it should have non-zero width). The corner's width should not depend on what comes before or after it, because that moves the spacing logic out of this single function.
I'll combine the isSquareCorner checks.
I think that once I start testing with more squared corners (stacked blocks) I'll have to change the square corner part of this as well, to have the slight difference between previous and next offsets.
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.
sgtm.
Tests passing: 65/109 -> 77/109 in LTR