Summary:

[WIP] Front end DnD Editor for per-user file storage

Review Request #5954 — Created June 8, 2014 and discarded Jan. 7, 2015, 3:05 p.m.

Information
Owner:	rvaiya
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

Ready for the first iteration of review.

Testing Done

Changed bodyTopEditor in the review dialog view to use the created view, dragged and dropped a few files.
Manually tried adding multiple files to a DnDEditorView
Observed and recorded expected behaviour when a file dragged over both a window that has a DnDEditorView in focus and one that does not.

Extensive testing won't be possible until the backend stuff has been implemented.

Issues

Description	From	Last Updated
RB.MarkdownFileView?	mike_conley	June 28, 2014, 1:23 p.m.
Just a nit - we tend to put the documentation for a function above the function header. It's also a …	mike_conley	June 28, 2014, 1:25 p.m.
What is a "file" in this context? Is it something that we'd be able to get a mimetype from? That …	mike_conley	June 28, 2014, 1:25 p.m.
No multiple drops?	mike_conley	June 28, 2014, 1:24 p.m.
This should use "true" and "false" rather than "1" and "0".	david	June 28, 2014, 9:30 p.m.
We always use brackets around conditional blocks: if (!this.active) { return; } In this specific case, because there's already a …	david	June 28, 2014, 9:30 p.m.
Indentation should be 4 spaces with no tabs. You should check the rest of your changes for proper indentation.	david	June 28, 2014, 9:30 p.m.
It's only really common for web browsers to show .gif, .png, and .jpg/jpeg. I don't see any need to support …	david	June 28, 2014, 9:30 p.m.
This doesn't seem to "append" for codemirror fields.	david	July 15, 2014, 3:15 p.m.
Care to put this above the method instead of inside it?	david	July 15, 2014, 3:15 p.m.
Indentation is off here.	david	July 15, 2014, 3:15 p.m.
I'm not sure what this comment means. Is this supposed to be a to-do item?	david	July 15, 2014, 3:15 p.m.
Add a space after //	david	July 15, 2014, 3:15 p.m.
What does this comment mean?	david	July 15, 2014, 3:15 p.m.
Can you add the word "TODO" here?	david	July 15, 2014, 3:15 p.m.
Can you put the comment above the method name?	david	July 15, 2014, 3:15 p.m.
Should be only one blank line. Please also add a comment for attachFileView	david	July 15, 2014, 3:15 p.m.
The outer parens can be removed here.	david	July 15, 2014, 3:15 p.m.
The var statement should be at the beginning of the function (to make the hoisting clear). This line needs some …	david	July 15, 2014, 3:15 p.m.
We should lower-case file.name before comparing, especially if someone is coming from a case-insensitive filesystem. It might also make sense …	david	July 15, 2014, 3:15 p.m.
Should have a space after //. This should also be marked 'TODO'	david	July 15, 2014, 3:15 p.m.
Should have a space after // (or just get rid of this--I think it's pretty clear that these are event …	david	July 15, 2014, 3:15 p.m.
Remove this line.	david	July 15, 2014, 3:15 p.m.
Backbone.Events should already be mixed in to any view.	david	July 15, 2014, 3:15 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/views/markdownEditorView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/views/markdownEditorView.js

Groups:

+	students

Description:

~		[WIP] Front end DnD Editor for per-user file storage
	~	Ready for the first iteration of review.

Testing Done:

~		Nothing yet
	~	Changed bodyTopEditor in the review dialog view to use the created view, dragged and dropped a few files. Extensive testing won't be possible until the backend stuff has been implemented.
-
-		Still firming out the details not ready for review yet.

Commit:

-	d5462fa2469d1573d6a9137c641dfebee1491971
+	9224d90c6e24d185b75a267e2f87c7252071cbc2

Diff:

Revision 2 (+104 -2)

Show changes

	reviewboard/static/rb/js/views/dndUploaderView.js
	reviewboard/static/rb/js/views/markdownEditorView.js
	reviewboard/static/rb/js/views/reviewDialogView.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/views/markdownEditorView.js
    reviewboard/static/rb/js/views/dndUploaderView.js

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

I'm curious, why the switch to the container over the body?

rvaiya June 16, 2014, 10:31 a.m.

This was done to prevent the existing review request DnD uploader from grabbing the focus in the modal review dialog. Considering that my solution should be a drop in replacement for any markdown editor it might be a better idea to limit the existing review request uploader to only operate on the review request editor (or even just the file specific portion of that view). Something like $('#review_request').

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

Show all issues

RB.MarkdownFileView?

rvaiya June 16, 2014, 10:31 a.m.

Since this view was created specifically for RB.DnDEditorView (and should only be owned by that view) I didn't want to make it "global".

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

Show all issues

Just a nit - we tend to put the documentation for a function above the function header. It's also a good idea to thoroughly describe the parameters to the function, and their expected types.

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

Show all issues

What is a "file" in this context? Is it something that we'd be able to get a mimetype from? That might be better than looking at file extensions.

rvaiya June 16, 2014, 10:31 a.m.

file is an instance of File. According to the MDN page pertaining directly to File there doesn't seem to be a way to retrieve the mime type of a file using a method or attribute. However according to this (https://developer.mozilla.org/en-US/docs/Using_files_from_web_applications) the file object should have a type attribute, however I don't know whether or not this is standard or if I should use it.

rvaiya June 28, 2014, 1:25 p.m.

Testing suggests that the type attribute is available in both recent versions of firefox and chrome, however I can't find any standard documentation describing it. As such I've kept the extension check as a fallback mechanism.

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 2)
Show all issues
```
No multiple drops?
```
1. RV
  
  rvaiya June 16, 2014, 10:31 a.m.
  Good point, I will fix this.

Testing Done:

~		Changed bodyTopEditor in the review dialog view to use the created view, dragged and dropped a few files. Extensive testing won't be possible until the backend stuff has been implemented.
	~	Changed bodyTopEditor in the review dialog view to use the created view, dragged and dropped a few files.
	+	Manually tried adding multiple files to a DnDEditorView
	+	Observed and recorded expected behaviour when a file dragged over both a window that has a DnDEditorView in focus and one that does not.
	+
	+	Extensive testing won't be possible until the backend stuff has been implemented.

Commit:

-	9224d90c6e24d185b75a267e2f87c7252071cbc2
+	0d61228847414833f006e944bfd2a794435e297e

Diff:

Revision 3 (+169 -3)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/views/dndUploaderView.js
	reviewboard/static/rb/js/views/markdownEditorView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/markdownEditorView.js
    reviewboard/static/rb/js/views/dndUploaderView.js



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

Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/markdownEditorView.js
    reviewboard/static/rb/js/views/dndUploaderView.js

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

Show all issues

This should use "true" and "false" rather than "1" and "0".

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

Show all issues

We always use brackets around conditional blocks:

if (!this.active) {
    return;
}


In this specific case, because there's already a conditional around the contents of the method, I'd just fold it into that:

if (this.action &&
    !this._dropOverlay &&
    ...

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 3)

Show all issues

Indentation should be 4 spaces with no tabs. You should check the rest of your changes for proper indentation.

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 3)

Show all issues

It's only really common for web browsers to show .gif, .png, and .jpg/jpeg. I don't see any need to support .tiff or other formats.

Commit:

-	0d61228847414833f006e944bfd2a794435e297e
+	a6d85254f019562a377a0cea0ba61457b60807c8

Diff:

Revision 4 (+172 -4)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/views/dndUploaderView.js
	reviewboard/static/rb/js/views/markdownEditorView.js
	reviewboard/static/rb/js/views/reviewDialogView.js
	reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/markdownEditorView.js
    reviewboard/static/rb/js/views/dndUploaderView.js



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

Ignored Files:
    reviewboard/static/rb/js/views/reviewDialogView.js
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/views/markdownEditorView.js
    reviewboard/static/rb/js/views/dndUploaderView.js

Fix it!

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

Show all issues

This doesn't seem to "append" for codemirror fields.

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Care to put this above the method instead of inside it?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Indentation is off here.
```

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

Show all issues

I'm not sure what this comment means. Is this supposed to be a to-do item?

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Add a space after //
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
What does this comment mean?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Can you add the word "TODO" here?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Can you put the comment above the method name?
```

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

Show all issues

Should be only one blank line. Please also add a comment for attachFileView

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
The outer parens can be removed here.
```

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

Show all issues

The var statement should be at the beginning of the function (to make the hoisting clear). This line needs some spaces in it:

function(file) {
    var i;

    ...

    for (i = 0; i < imageExtensions.length; i++) {

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

Show all issues

We should lower-case file.name before comparing, especially if someone is coming from a case-insensitive filesystem.

It might also make sense to use an indexOf call instead of regex match, for a slight efficiency gain.

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Should have a space after //. This should also be marked 'TODO'
```

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

Show all issues

Should have a space after // (or just get rid of this--I think it's pretty clear that these are event handlers.

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Remove this line.
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
Show all issues
```
Backbone.Events should already be mixed in to any view.
```

Review Board 6.0 beta 3 (dev)

[WIP] Front end DnD Editor for per-user file storage

Groups:

Description:

Testing Done:

Commit:

Diff:

Testing Done:

Commit:

Diff:

Commit:

Diff:

Status: Discarded