-
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
Propagate the visible state when blocks connect #2003
Propagate the visible state when blocks connect #2003
Conversation
79b7dc0
to
fc087ed
Compare
This still needs more work. It seems to be working, but there were quite a few edge cases that I had to fix and I don't think I've caught them all. |
Four lint errors in
|
fc087ed
to
9263f4e
Compare
|
||
|
||
function renderedConnectionTest_setup() { | ||
|
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.
Intending to complete these before merging?
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.
Yes, I need to write tests still. Probably won't happen for a week or two.
|
||
/** | ||
* Disconnect this connection. | ||
*/ |
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.
@override
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.
Blockly.RenderedConnection.prototype.connect = function(otherConnection) { | ||
Blockly.RenderedConnection.superClass_.connect.call(this, otherConnection); | ||
// This is a quick check to make sure we aren't doing unecessary work. | ||
if (this.hidden_ || otherConnection.hidden_) { |
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 there actually a need to test the inferior connection? I can't think of one, though test both does seem safer.
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.
It's easier to just check both rather than verify which is the inferior connection.
This fixes google#1967. In rendered connections when connecting: - If the superior connection is hidden this hides the newly connected block. - If the superior connection isn't hidden it makes sure the block is visible. In rendered connections when disconnecting: - If the superior connection is hidden, make the disconnected block stack visible. TODO before review: - write tests. - update collapsed message
9263f4e
to
b071b4f
Compare
Tests added! This PR is now ready to go once there's a final review. |
ea86e22
to
907d312
Compare
907d312
to
39e6bf2
Compare
This reverts commit ec78eeb.
Revert changes related to #2003
This reverts commit ec78eeb.
This fixes #1967.
In rendered connections when connecting:
In rendered connections when disconnecting:
TODO before review: write tests.
The basics
The details
Resolves
#1967
Proposed Changes
Adds logic to rendered_connection to hide or unhide blocks when they are connected to a hidden connection.
Reason for Changes
If you programatically connect a block to a hidden connection the block remains visible and gets placed in a random spot. The block should be hidden when connected and unhidden when disconnected.
Test Coverage
Tested on:
Desktop Chrome
Additional Information
This change is