yyoud88 got a fish trophy!
Make regionCommentBlock movable.
Review Request #6886 — Created Feb. 1, 2015 and discarded
| Information | |
|---|---|
| yyoud88 | |
| Review Board | |
| master | |
| Reviewers | |
| reviewboard, students | |
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
|
-
-
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 1) Style nitpick: blank line between a statement and a block
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) You can use
_.bind, .e.gcommentBlockView.setSelectionRegionSize( _.bind(function() { ... }, this); -
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Unindent the comment relative to the
*. Same below -
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Our max line length is 79 so take full advantage of it for comments. Same elsewhere.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) The second line should line up with the open quote on the first line, i.e.
this.model.on('foo...', this._bar, ...) -
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Blank line between a statement and a block
YY
Change Summary:
improve styles.
Commit: |
|
||||
|---|---|---|---|---|---|
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: |
|
||||
|---|---|---|---|---|---|
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
-
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) In case you were wondering, this is only available to views, and only works for DOM events.
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) What happens when this is fired the very first time for a new comment? Do we save that comment?
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) Since this will also need to be used to decide whether a resize can be performed, I'd suggest calling this
canUpdateBounds. -
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) These will all need doc comments in the form used in
parse()above. -
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/models/regionCommentBlockModel.js (Diff revision 1) 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. -
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 1) 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.
-
reviewboard/static/rb/js/views/abstractCommentBlockView.js (Diff revision 1) Blank lines on either side of the conditional, like:
this._updateTooltip(); if (options.notify) { ... } RB.DraftReview.... -
-
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) JavaScript doesn't allow a final trailing comma. It'll break IE.
-
reviewboard/static/rb/js/views/imageReviewableView.js (Diff revision 1) Blank line here. Both these statements are doing very different things, and much like paragraphs covering different topics, they should not be grouped together.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Hmmm, there's probably an easier way to do this. I think you want:
events: _.defaults({ 'mousedown .selection-flag': '...', ..., }, _super(this).events) -
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Instead of these, you can do:
_.bindAll(this, '_onDrag', '_onWindowMouseUp');It'll replace those functions with bound versions.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) This doesn't need to be indented.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Not sure these are necessary. I'd just put the logic in the calling functions.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) When possible, single quotes are preferred over double quotes in both Python and JavaScript.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) 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.
-
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Blank line before this.
You can set multiple attributes at once by passing a dictionary.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) This doesn't describe what the function does.
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) Make sure the blank line in the comment is removed.
-
-
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) You should use
this.listenTo(...)instead ofthis.model.on(...)when listening to events. This helps with managing circular references in signal handlers. -
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 1) All the same comments here as above.
-
YY
Change Summary:
solved issues opened.
improved styles, method names, keys, ...
Commit: |
|
||||
|---|---|---|---|---|---|
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: |
|
|||||||||
|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||
Testing Done: |
|
YY
Description: |
|
|---|
-
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?
-
reviewboard/static/rb/js/views/regionCommentBlockView.js (Diff revision 4) These lines look too long (wrap to 80 columns).
YY
Change Summary:
fixed style issue
Commit: |
|
||||
|---|---|---|---|---|---|
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: |
|
|---|
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