-
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
Cleanup for in functionality #2423
Conversation
tests/mocha/astnode_test.js
Outdated
var inNode = node.in(); | ||
assertEquals(inNode, null); | ||
}); | ||
test('fromInputToNext', function() { |
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 actually fromInputToPrevious? It looks like it's going from a statement input to the previous connection on the first block in the substack.
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.
Yeah it should be fromInputToPrevious. Just changed the name
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.
Looks reasonable. I have one question for clarification.
core/ast_node.js
Outdated
@@ -474,6 +471,31 @@ Blockly.ASTNode.prototype.getOutAstNodeForBlock_ = function(block) { | |||
} | |||
}; | |||
|
|||
/** | |||
* Find the first editable field or input with a connection on a given block. | |||
* @param {Blockly.BlockSvg} block The source block of the current location. |
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.
block can't be null (block.inputList would fail), so this should be {!Blockly.BlockSvg}
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.
Fixed
core/ast_node.js
Outdated
Blockly.ASTNode.prototype.findFirstFieldOrInput_ = function(block) { | ||
var inputs = block.inputList; | ||
for (var i = 0; i < inputs.length; i++) { | ||
var input = inputs[i]; |
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.
Generally we use this form:
for (var i = 0, input; input = inputs[i]; i++) {
Here and in the j loop below.
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.
Fixed
@@ -250,7 +250,7 @@ Blockly.ASTNode.prototype.findNextEditableField_ = function(location, | |||
var fieldRow = parentInput.fieldRow; | |||
var fieldIdx = fieldRow.indexOf(location); | |||
var startIdx = opt_first ? 0 : fieldIdx + 1; | |||
for (var i = startIdx; i < fieldRow.length; i++) { | |||
for (var i = startIdx, field; field = fieldRow[i]; i++) { | |||
var field = fieldRow[i]; |
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 line is redundant. :)
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.
Shoot missed that. Will fix in my next PR.
The basics
The details
Resolves
Proposed Changes
Fixes:
Reason for Changes
Test Coverage
Adds tests in mocha
Tested on:
Additional Information