fix: issue 6002 #6011
fix: issue 6002 #6011
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
There was a problem hiding this comment.
Perfect solution! Thank you for identifying the problem and contributing a fix back to core :D
There are a few things to sort out with regard to the unit tests you added. But overall this is a lovely PR.
Also note that you'll have to sign the CLA before we can merge your code :/
Thank you again!
tests/mocha/comment_test.js
Outdated
| }); | ||
| function assertComment(workspace, text) { | ||
| const count = workspace.getAllBlocks().length; | ||
| // show comment |
There was a problem hiding this comment.
Start with a capital and end with a period =) (Here and below)
| // show comment | |
| // Show comment. |
There was a problem hiding this comment.
Fixed as suggested.
There was a problem hiding this comment.
Thank you for all the fixes!
tests/mocha/comment_test.js
Outdated
| }); | ||
| test('Redo', function() { | ||
| // undo & undo | ||
| this.workspace.undo(false); |
There was a problem hiding this comment.
Thank you so much for adding unit tests :D But the way these are structured needs to be changed a bit. Right now this test is relying on the state of the previous test, which means they can't be run on their own or in parallel.
Since this is more of an integration test (it tests a bunch of systems and how they "integrate", rather than just one "unit") you could make it one large test, rather than breaking it out into separate ones. Or even better, instead of writing an automated test, you could document your test steps in a comment on this PR, and we can add the steps to our manual testing document =)
There was a problem hiding this comment.
Thank you for your advice.
Which action do you think is better?
- Remove the added test code and document the test steps in this PR.
- Separate the added test code into another file (e.g. comment_deserialization_test.js) and modify the structure.
There was a problem hiding this comment.
I gave it a try to separate the test code and modify the structure.
There was a problem hiding this comment.
Awesome! I pulled down the tests and now they all work when run in isolation. Thank you for the fixes :D
|
Thank you for your reply! I've just signed the CLA. |
fix: Modify test structure.
|
Thanks for signing the CLA @yamadayutaka :D I'll review your changes Monday =)
Also, this is rad for your first contribution! |
|
Thank you for putting up a PR for this! I really appreciate that you put in tests, especially for your first contribution. We always want to improve our test suite, and having contributors submit tests is super helpful =) If you want to contribute more we have help-wanted issues in this repository. Or even better, we have help-wanted issues, and good-first-issues in blockly/samples. That is the repository where we put extensions/plugins to Blockly, so it's a bit easier to develop in =) If you're not interested in contributing any more right now, that's totally cool too! We still really appreciate you putting in this fix. Best wishes! 🌻 |
|
Thank you for reviewing my PR. |
The basics
The details
Resolves
#6002
Proposed Changes
Some comment attributes (pinned/height/width) is removed when block is deleted ( in the trashcan ).
But BlockSerializer is always restore all attributes then comment bubble size may become NaN (and error occuered).
So I added a check when deserializing.
Reason for Changes
Fixing bugs.
Test Coverage
I wrote mocha test in tests/mocha/comment_test.js.
Documentation
Additional Information