yyoud88 got a fish trophy!
Make regionCommentBlock movable.
Review Request #6886 — Created Feb. 1, 2015 and discarded
Made draft RegionCommentBlock movable, by drag-and-drop.
Currently, a RegionCommentBlock is not movable, even if it is a draft. This change makes draft RegionCommentBlocks movable by drag-and-drop. Users finish moving it by mouse-up action, and the draft RegionCommentBlock's new position is saved on server.
Passed all tests in "/js-test/".
Browser test done:
1. Go to file attachment review page, for example, http://localhost:8080/r/7/file/4/
2. Make draft comment
3. Try moving
4. Refresh the page
5. See if the bounds are updatedEdge cases tested:
1. Open file attachment review page, make move "once" and refresh. => Working well. Confirmed the location is updated.
2. Try click-and-drag non-draft commentBlocks. => Non-draft commentBlcoks are not movable. Working as expected.
| Description | From | Last Updated |
|---|---|---|
|
Blank line between these. |
 chipx86
|
|
|
What happens when this is fired the very first time for a new comment? Do we save that comment? |
chipx86
|
|
|
Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this … |
chipx86
|
|
|
These will all need doc comments in the form used in parse() above. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
notify is both pretty generic (in terms of option names) and yet too specific (in terms of the action that … |
chipx86
|
|
|
Generally-speaking, you'd want to remove this and change the below code to: if (options.notify !== false) { ... } However, … |
chipx86
|
|
|
Blank lines on either side of the conditional, like: this._updateTooltip(); if (options.notify) { ... } RB.DraftReview.... |
chipx86
|
|
|
Style nitpick: blank line between a statement and a block |
 brennie
|
|
|
You can use _.bind, .e.g commentBlockView.setSelectionRegionSize( _.bind(function() { ... }, this); |
brennie
|
|
|
Blank line between these. |
chipx86
|
|
|
Blank line between these. |
chipx86
|
|
|
JavaScript doesn't allow a final trailing comma. It'll break IE. |
chipx86
|
|
|
Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should … |
chipx86
|
|
|
Hmmm, there's probably an easier way to do this. I think you want: events: _.defaults({ 'mousedown .selection-flag': '...', ..., }, … |
chipx86
|
|
|
Instead of these, you can do: _.bindAll(this, '_onDrag', '_onWindowMouseUp'); It'll replace those functions with bound versions. |
chipx86
|
|
|
This doesn't need to be indented. |
chipx86
|
|
|
Not sure these are necessary. I'd just put the logic in the calling functions. |
chipx86
|
|
|
When possible, single quotes are preferred over double quotes in both Python and JavaScript. |
chipx86
|
|
|
No indentation like this. Make sure to follow the doc conventions throughout the codebase. One line summary, multi-line description. This … |
chipx86
|
|
|
Unindent the comment relative to the *. Same below |
brennie
|
|
|
Blank line here. |
chipx86
|
|
|
Blank line before this. You can set multiple attributes at once by passing a dictionary. |
chipx86
|
|
|
Our max line length is 79 so take full advantage of it for comments. Same elsewhere. |
brennie
|
|
|
This doesn't describe what the function does. |
chipx86
|
|
|
Make sure the blank line in the comment is removed. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
And here. |
chipx86
|
|
|
The second line should line up with the open quote on the first line, i.e. this.model.on('foo...', this._bar, ...) |
brennie
|
|
|
You should use this.listenTo(...) instead of this.model.on(...) when listening to events. This helps with managing circular references in signal handlers. |
chipx86
|
|
|
Blank line between a statement and a block |
brennie
|
|
|
All the same comments here as above. |
chipx86
|
|
|
Blank line here. |
chipx86
|
|
|
These lines look too long (wrap to 80 columns). |
 david
|
YY
- Change Summary:
-
improve styles.
- Commit:
-
3ce32bc39735c43e42a44a35992e9618ba4940b489a08c16b6db43d7cf789d5721e3598520a979de
- Diff:
-
Revision 2 (+262 -21)
-
Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js
YY
- Change Summary:
-
fixed style issues
- Commit:
-
89a08c16b6db43d7cf789d5721e3598520a979de5b78dd6c9dab603d8836d118e24056c73cc73488
- Diff:
-
Revision 3 (+262 -21)
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js
-
-
-
-
-
Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this
canUpdateBounds. -
-
-
-
notifyis both pretty generic (in terms of option names) and yet too specific (in terms of the action that the model is trying to block). Models should never have a concept of UI, and signal emitters should not assume how a signal will be used.Instead of an option name suggesting behavior, it should instead suggest what happened. So, I'd go with
boundsUpdated: true. Then the view (or any future code) could make smart decisions based on the knowledge that the bounds have been updated. -
Generally-speaking, you'd want to remove this and change the below code to:
if (options.notify !== false) { ... }However, with the change I suggested to the name, which also flips the value, you won't need either.
-
Blank lines on either side of the conditional, like:
this._updateTooltip(); if (options.notify) { ... } RB.DraftReview.... -
-
-
-
Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should not be grouped together.
-
Hmmm, there's probably an easier way to do this. I think you want:
events: _.defaults({ 'mousedown .selection-flag': '...', ..., }, _super(this).events) -
Instead of these, you can do:
_.bindAll(this, '_onDrag', '_onWindowMouseUp');It'll replace those functions with bound versions.
-
-
-
-
No indentation like this.
Make sure to follow the doc conventions throughout the codebase. One line summary, multi-line description.
This applies to all comments in your change.
-
-
-
-
-
-
-
You should use
this.listenTo(...)instead ofthis.model.on(...)when listening to events. This helps with managing circular references in signal handlers. -
-
YY
- Change Summary:
-
solved issues opened.
improved styles, method names, keys, ... - Commit:
-
5b78dd6c9dab603d8836d118e24056c73cc73488784c8ead1e8e5343c2a933c372222fe228d6e297
- Diff:
-
Revision 4 (+259 -21)
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js
-
Hey Stanley, could you please update the Description and Testing Done fields based on https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ ?
YY
- Change Summary:
-
updated "description" and "testing done" part.
- Summary:
-
[WIP] Make regionCommentBlock movable.Make regionCommentBlock movable.
- Description:
-
~ Allow draft regionCommentBlock to be moved.
~ Made draft RegionCommentBlock movable, by drag-and-drop.
+ A user finishes moving it by mouse-up, and it will update new position on server. - Testing Done:
-
~ passed all tests in http://localhost:8080/js-tests/
~ Passed all tests in http://localhost:8080/js-tests/
+ Tested on my local machine on browsers.
YY
- Description:
-
~ Made draft RegionCommentBlock movable, by drag-and-drop.
~ A user finishes moving it by mouse-up, and it will update new position on server. ~ Made draft RegionCommentBlock movable, by drag-and-drop.
~ + Currently, a RegionCommentBlock is not movable, even if it is a draft. This change makes draft RegionCommentBlocks movable by drag-and-drop. Users finish moving it by mouse-up action, and the draft RegionCommentBlock's new position is saved on server.
-
Please be specific about exactly what manual tests you executed in your browser. What test cases did you try? What were the results of each test? In particular, this sort of interaction often has some edge cases--which edge cases have you identified, and are they handled correctly?
-
YY
- Change Summary:
-
fixed style issue
- Commit:
-
784c8ead1e8e5343c2a933c372222fe228d6e2979fcb612dc59c1fdf140d4e6b4a0d652315391db9
- Diff:
-
Revision 5 (+264 -21)
-
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js
YY
- Testing Done:
-
~ Passed all tests in http://localhost:8080/js-tests/
~ Tested on my local machine on browsers. ~ Passed all tests in "/js-test/".
~ + Browser test done:
+ 1. Go to file attachment review page, for example, http://localhost:8080/r/7/file/4/ + 2. Make draft comment + 3. Try moving + 4. Refresh the page + 5. See if the bounds are updated + + Edge cases tested:
+ 1. Open file attachment review page, make move "once" and refresh. => Working well. Confirmed the location is updated. + 2. Try click-and-drag non-draft commentBlocks. => Non-draft commentBlcoks are not movable. Working as expected.
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/views/abstractCommentBlockView.js reviewboard/static/rb/js/resources/models/tests/baseResourceModelTests.js reviewboard/static/rb/js/models/regionCommentBlockModel.js reviewboard/static/rb/js/views/regionCommentBlockView.js reviewboard/static/rb/js/views/imageReviewableView.js reviewboard/static/rb/js/resources/models/baseResourceModel.js