Refactor top row #2472
Refactor top row #2472
Conversation
| @@ -127,6 +127,8 @@ Blockly.BlockRendering.RenderInfo.prototype.measure_ = function() { | |||
| */ | |||
| Blockly.BlockRendering.RenderInfo.prototype.createRows_ = function() { | |||
| var activeRow = new Blockly.BlockRendering.Row(); | |||
| this.topRow = this.createTopRow_(); | |||
There was a problem hiding this comment.
Move these above the activeRow line, so that it's clear that the activeRow will come after these.
Do we still need this.topRow?
There was a problem hiding this comment.
We use it in the draw function and draw highlight function so I think it is probably worth it to keep it around? It seems cleaner than getting the first element in the list of rows every time.
| /** | ||
| * Create the top row and fill the elements list with the correct measurables | ||
| * based on values from the block. | ||
| * @return {Blockly.BlockRendering.TopRow} The top row with all the elements |
There was a problem hiding this comment.
Why return? I expected you to just set this.topRow inside this function.
There was a problem hiding this comment.
With all non-spacer elements created.
| @@ -292,6 +337,23 @@ Blockly.BlockRendering.RenderInfo.prototype.getInRowSpacing_ = function(prev, ne | |||
| } | |||
| } | |||
|
|
|||
| // Spacing between a hat and a corner | |||
| if (prev.isRoundedCorner() || prev.isSquareCorner()) { | |||
There was a problem hiding this comment.
Shouldn't even have a rounded corner preceding a hat.
| @@ -356,7 +418,11 @@ Blockly.BlockRendering.RenderInfo.prototype.alignRowElements_ = function() { | |||
| var desiredWidth = this.width; | |||
| var missingSpace = desiredWidth - currentWidth; | |||
| if (missingSpace) { | |||
| this.addAlignmentPadding_(row, missingSpace); | |||
| if (row.type === 'top row') { | |||
| row.getLastSpacer().width += missingSpace; | |||
There was a problem hiding this comment.
This looks like it belongs in addAlignmentPadding.
| @@ -26,6 +26,15 @@ Blockly.BlockRendering.Measurable.prototype.isField = function() { | |||
| return this.type == 'field'; | |||
| }; | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
Note for later: we may remove these helper functions if all of them end up being direct checks against types.
There was a problem hiding this comment.
Made a gnosis ticket in cleanup section and added todo
| */ | ||
| Blockly.BlockRendering.PreviousConnection = function() { | ||
| Blockly.BlockRendering.PreviousConnection.superClass_.constructor.call(this); | ||
| this.renderRect = null; |
There was a problem hiding this comment.
If you're not using renderRect, remove it. On render/collab it's only ever set to null, and never used.
| Blockly.BlockRendering.PreviousConnection.superClass_.constructor.call(this); | ||
| this.renderRect = null; | ||
| this.type = 'previous connection'; | ||
| this.height = BRC.MEDIUM_PADDING; |
There was a problem hiding this comment.
Add a new constant for this height. BRC.STACK_CONNECTION_HEIGHT, maybe? Use it here and for NextConnection, and set it equal to BRC.MEDIUM_PADDING. Changing connection height/shapes is a common tweak for rendering.
There was a problem hiding this comment.
Or `NOTCH_HEIGHT, since we seem to be calling those 'notches'
| Blockly.BlockRendering.Hat.superClass_.constructor.call(this); | ||
| this.renderRect = null; | ||
| this.type = 'hat'; | ||
| this.height = BRC.MEDIUM_PADDING; |
There was a problem hiding this comment.
Add a separate constant for this height as well.
There was a problem hiding this comment.
...though actually, this isn't the height up, right? It's the height down into the row? If so, does this need to have any height at all, or can we say its height is 0?
| Blockly.BlockRendering.RoundCorner.superClass_.constructor.call(this); | ||
| this.renderRect = null; | ||
| this.type = 'round corner'; | ||
| this.width = 8; |
| Blockly.BlockRendering.TopRow = function(block) { | ||
| Blockly.BlockRendering.TopRow.superClass_.constructor.call(this); | ||
|
|
||
| this.elements = []; |
There was a problem hiding this comment.
this.elements has already been created in the Row constructor.
|
|
||
| if (squareCorner) { | ||
| this.topRow.elements.push( | ||
| new Blockly.BlockRendering.SquareCorner(this.width)); |
There was a problem hiding this comment.
Get rid of unused width, here and on other measurables
The basics
The details
Resolves
Proposed Changes
Refactors the top row to inherit from Row instead of Measurable. Top row is now treated like a row and has an elements list with measurables such as corners, previous connection, and hats.
Reason for Changes
It makes more sense for the top row to be treated like a row.
Test Coverage
Tested on:
Additional Information