Summary

Drag 'n Drop Inline Images Frontend

Review Request #6510 — Created Oct. 26, 2014 and discarded May 6, 2016, 1:45 p.m.

Information

Owner

dkus

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard, students

People

Description

Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.

These changes focus on the frontend work and include:

Extending the DnDUploaderView to support multiple overlays on the page. Multiple DropTargets can be registered with the DnDOverlay instance.
Extending the TextEditorView to support dragging and dropping images
Adding a resource for UserFileAttachments

Testing Done

Tested in Chrome 38.0.2125.122, Firefox 32.0.2, Safari 8.0, Internet Explorer 11.0.9600

All existing unit tests pass.

Added tests for the changes done to the TextEditorView (testing the appendText method and the Drag and Drop functionality).
Added tests for serialization and parsing of the UserFileAttachment resource.
Added tests for the DnDUploader functionality.

Files

Issues

Description	From	Last Updated
It might be better to use the / / multi line comments here	ML mloyzer	Nov. 15, 2014, 9:14 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:41 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:44 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:46 a.m.
Variable definitions should all be the first thing in the method.	david	Nov. 23, 2014, 10:47 a.m.
Can you leave this as-is?	david	Nov. 23, 2014, 10:46 a.m.
I'd suggest defining a $window as the first thing, and then using that for both. That way it doesn't have …	david	Nov. 23, 2014, 10:48 a.m.
Wrap to 80 column.	david	Nov. 23, 2014, 10:51 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:51 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:52 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:52 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:52 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:52 a.m.
Wrap to 80 columns.	david	Nov. 23, 2014, 10:52 a.m.
Nit - let's put these in the same order as the function args.	mike_conley	Nov. 28, 2014, 5:08 p.m.
Might as well throw bmp, tiff and svg in there too.	mike_conley	Nov. 28, 2014, 5:09 p.m.
This can be: return imageExtensions.some(function(extension) { return file.name.match(new RegExp("\\." + extension + "$", "i")); }); That'll work in IE9 and …	mike_conley	Nov. 28, 2014, 5:13 p.m.
Nit "and" -> "an"	mike_conley	Nov. 28, 2014, 5:13 p.m.
Even though this is supposed to be a very cheap request, synchronous XHR is best to be avoided, since it …	mike_conley	Nov. 28, 2014, 5:43 p.m.
We probably need some error handling here in case things go wrong with the upload.	mike_conley	Nov. 30, 2014, 5:27 p.m.
Where are these magic 6's coming from?	mike_conley	Nov. 28, 2014, 6:13 p.m.
Ah - is there were the magic 6 comes from? Can we apply an attribute or class on the element …	mike_conley	Nov. 28, 2014, 6:13 p.m.
Might be good to get this stuff in CSS instead - perhaps a class for the window overlay. Or a …	mike_conley	Nov. 28, 2014, 6:13 p.m.
This seems like it wants to be a model but doesn't want to adhere to model conventions. Models work with …	chipx86	Dec. 4, 2014, 6:15 p.m.
This doesn't buy us anything that new already gives us. It's actually nicer to pass in the arguments as a …	chipx86	Dec. 4, 2014, 8:10 a.m.
Blank line between these.	chipx86	Dec. 4, 2014, 7:49 a.m.
And here.	chipx86	Dec. 4, 2014, 7:49 a.m.
Two blank lines.	chipx86	Dec. 4, 2014, 7:50 a.m.
Blank line between these.	chipx86	Dec. 4, 2014, 7:50 a.m.
Most of these can go inline below, saving some lines of code.	chipx86	Dec. 4, 2014, 7:51 a.m.
This can break down the road if styles are changed. Instead, use outerWidth() and outerHeight(), which account for the border.	chipx86	Dec. 4, 2014, 6 p.m.
This should be a single .css({ ... }) statement.	chipx86	Dec. 4, 2014, 7:54 a.m.
The var should go at the top of the file.	chipx86	Dec. 4, 2014, 7:55 a.m.
Same comments here as above.	chipx86	Dec. 4, 2014, 6:05 p.m.
No blank line here.	chipx86	Dec. 4, 2014, 7:56 a.m.
Two blank lines.	chipx86	Dec. 4, 2014, 7:56 a.m.
Not every page will need this. Is it not sufficient to create an instance in ReviewRequestEditorView, or any other pages …	chipx86	Dec. 4, 2014, 6:48 p.m.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/views/markdownEditorView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/views/markdownEditorView.js

Change Summary:: Adding group 'students' to reviewers
Groups:: students

Is this still valid, or has it been replaced by /r/6454/?

dkus Oct. 27, 2014, 8:08 a.m.

This is indeed still valid. These changes cover the frontend work, the other review request is for the backend.

Commit:

28140039d5a18152303a4f29aaef66f82ac01af6

Diff:

Revision 2 (+299 -9)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/models/userFileModel.js
	reviewboard/static/rb/js/resources/models/apiTokenModel.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/views/commentDialogView.js
	reviewboard/static/rb/js/views/dndUploaderView.js
	reviewboard/static/rb/js/views/markdownEditorView.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/models/userFileModel.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/views/markdownEditorView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/models/userFileModel.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/commentDialogView.js
    reviewboard/static/rb/js/views/markdownEditorView.js

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

It might be better to use the / / multi line comments here

Change Summary:

Working toward allowing a user to drag and drop onto multiple areas of the window.

Diff:

Revision 3 (+338 -39)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/models/userFileModel.js
	reviewboard/static/rb/js/pages/views/pageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/resources/models/apiTokenModel.js
	4 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/models/userFileModel.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/pageView.js
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/models/userFileModel.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/pages/views/pageView.js
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/resources/models/apiTokenModel.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Change Summary:: Some screenshots of how the dnd overlays look right now. The text in the overlay needs to be moved (we don't really want the text from two different dnd overlays to overlap).
Added Files:

Change Summary:

More work on the DnDUploader view. Added many tests and updated description and testing done.

Summary:

[WIP] Drag 'n Drop Inline Images Frontend

Drag 'n Drop Inline Images Frontend

Description:

		Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.

		These changes focus on the frontend work and include:

~		Extending the Markdown Editor View to support dragging and dropping images
~
~		Still a work in progress.
	~	Extending the DnDUploaderView to support multiple overlays on the page. Multiple DropTargets can be registered with the DnDOverlay instance.
	~	Extending the TextEditorView to support dragging and dropping images
	~	Adding a resource for UserFileAttachments

Testing Done:

~		WIP
	~	Tested in Chrome 38.0.2125.122, Firefox 32.0.2, Safari 8.0
	+
	+	All existing unit tests pass.
	+
	+	Added tests for the changes done to the TextEditorView (testing the appendText method and the Drag and Drop functionality).
	+	Added tests for serialization and parsing of the UserFileAttachment resource.
	+	Added tests for the DnDUploader functionality.

Diff:

Revision 4 (+567 -37)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	reviewboard/static/rb/js/ui/views/textEditorView.js
	reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Variable definitions should all be the first thing in the method.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you leave this as-is?
```

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 4)

The issue has been resolved. Show all issues

I'd suggest defining a $window as the first thing, and then using that for both. That way it doesn't have to run the jquery selector twice.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 column.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```
reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Wrap to 80 columns.
```

Change Summary:

Review request changes. Mostly line wrapping and also making sure the jquery selector is not run an unnecessary amount of times in dndUploaderView.js

Diff:

Revision 5 (+590 -36)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	reviewboard/static/rb/js/ui/views/textEditorView.js
	reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Change Summary:: Tested in Internet Explorer
Testing Done:: ~
Tested in Chrome 38.0.2125.122, Firefox 32.0.2, Safari 8.0
~
Tested in Chrome 38.0.2125.122, Firefox 32.0.2, Safari 8.0, Internet Explorer 11.0.9600

All existing unit tests pass.

Added tests for the changes done to the TextEditorView (testing the appendText method and the Drag and Drop functionality).

Added tests for serialization and parsing of the UserFileAttachment resource.

Added tests for the DnDUploader functionality.

reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Nit - let's put these in the same order as the function args.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Might as well throw bmp, tiff and svg in there too.
```

reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)

The issue has been resolved. Show all issues

This can be:
return imageExtensions.some(function(extension) {
  return file.name.match(new RegExp("\\." + extension + "$", "i"));
});
That'll work in IE9 and up.

reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Nit "and" -> "an"
```

reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)

The issue has been resolved. Show all issues

Even though this is supposed to be a very cheap request, synchronous XHR is best to be avoided, since it 100% blocks JS execution. If there are network problems (say, our DNS server is flaking out on us), this can absolutely lock up everything in the user's tab (or their entire browser).

Is there a way of doing this asynchronously, and passing in a callback to do the upload of the whole file? Also, what happens if that first save fails?

reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)

The issue has been resolved. Show all issues

We probably need some error handling here in case things go wrong with the upload.

dkus Nov. 28, 2014, 5:48 p.m.

What would you recommend happen if things go wrong? Also, it can fail at 2 places (when doing the first POST and then after when updating with the file), should we handle both errors?

mike_conley Nov. 28, 2014, 6:39 p.m.

Good question. Our error reporting isn't very consistent - at times, we show an error that can be unfolded from the "Loading" spinner when we're updating a review request. Other times, we pop up an alert (ugh). Othertimes, there's inline error reporting (like when you try to set a reviewer on a review request to a username that does not exist).

Do we show any kind of progress or spinner while the upload is occurring? Ideally, the error would be located in or around there.

dkus Nov. 28, 2014, 8:09 p.m.

We don't show any progress / spinner while uploading right now. It's something that we decided to cut out so that I could have something finished before the pencils down date.

mike_conley Nov. 28, 2014, 8:19 p.m.

Ah, right. In that case, we should at least log it to the error console, and then follow up bugs for more robust error reporting.

dkus Nov. 29, 2014, 8:55 a.m.

Alright. Should I just use console.error()? I don't see that being used anywhere in the code base.

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
Where are these magic 6's coming from?
```

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 5)

The issue has been resolved. Show all issues

Ah - is there were the magic 6 comes from?
Can we apply an attribute or class on the element instead, and have a selector to apply the border? Putting non-positional styling in JS should be avoided if possible.

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 5)

The issue has been resolved. Show all issues

Might be good to get this stuff in CSS instead - perhaps a class for the window overlay. Or a separate class for the text input overlays.

Change Summary:

Moving the dnd-overlay styling into less file. Making the initial save for userFileAttachment run async. + Misc changes from code review.

Diff:

Revision 6 (+578 -37)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	reviewboard/static/rb/js/ui/views/textEditorView.js
	reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Change Summary:

Showing an alert() when the file attachment upload fails.

Diff:

Revision 7 (+585 -37)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	reviewboard/static/rb/js/ui/views/textEditorView.js
	reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Fix it!

Looking good. Got some things to fix up that are largely stylistic or JavaScript/jQuery/Backbone-isms that will help reduce code and logic.

reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 7)

The issue has been resolved. Show all issues

This seems like it wants to be a model but doesn't want to adhere to model conventions.

Models work with attributes instead of direct properties (like target). It also uses defaults for any default options.

You probably want to fill defaults with default values for target, dropAction, context, and active. You can then remove the initialize call.

Consumers will then need to access this data using get().

This is also missing documentation.

reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 7)

The issue has been resolved. Show all issues

This doesn't buy us anything that new already gives us. It's actually nicer to pass in the arguments as a dictionary instead of positional, since it's more self-documenting.

reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
And here.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Two blank lines.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Blank line between these.
```

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)

The issue has been resolved. Show all issues

Most of these can go inline below, saving some lines of code.

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)

An issue was opened. Show all issues

This can break down the road if styles are changed. Instead, use outerWidth() and outerHeight(), which account for the border.

dkus Dec. 4, 2014, 6:05 p.m.

This won't really work for my case. It's not the target that has the borders, it's the element who's height/width I'm setting. I would need to do something like setOuterWidth() and setOuterHeight() (I don't think those exist). Just setting the height of the element doesn't include it's borders, so with the borders it exceeds the height of the target.

chipx86 Dec. 4, 2014, 6:16 p.m.

Ahh. What you can do then is:

var width = target.width() + this.$el.getExtents('b', 'lr'),
    height = target.height() + this.$el.getExtents('b', 'tb');


That will get the total border size for the proper dimensions on this.$el add that to the target width/height.

dkus Dec. 4, 2014, 7:01 p.m.

Works fine in Firefox but doesn't seem to work in Chrome (returning 0 instead of 6).

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
This should be a single .css({ ... }) statement.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
The var should go at the top of the file.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
An issue was opened. Show all issues
```
Same comments here as above.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
No blank line here.
```
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Two blank lines.
```

reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 7)

The issue has been resolved. Show all issues

Not every page will need this. Is it not sufficient to create an instance in ReviewRequestEditorView, or any other pages that need this?

dkus Dec. 4, 2014, 6:05 p.m.

What if I create it in the ReviewablePageView?

chipx86 Dec. 4, 2014, 6:16 p.m.
```
That works fine too.
```

Change Summary:

Updating DnDDropTarget model to follow Backbonejs model conventions. Some formatting changes.

Diff:

Revision 8 (+618 -38)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	5 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Change Summary:

Disable the drag 'n drop overlay when the comment dialog has been closed.

Diff:

Revision 9 (+629 -39)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/models/dndDropTarget.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
	reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
	5 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/models/dndDropTarget.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
    reviewboard/static/rb/js/views/dndUploaderView.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/js/ui/views/textEditorView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Status:: Discarded
Change Summary:: Discarded in favor of /r/8147/