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)

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)
The issue has been resolved. 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)

The issue has been resolved. Show all issues

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

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

The issue has been resolved. 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)

The issue has been resolved. 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)

The issue has been resolved. 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)

An issue was opened. Show all issues

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

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

An issue was opened. Show all issues

Care to put this above the method instead of inside it?

reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. Show all issues
```
Indentation is off here.
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. 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)
An issue was opened. Show all issues
```
Add a space after //
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. Show all issues
```
What does this comment mean?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. Show all issues
```
Can you add the word "TODO" here?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. Show all issues
```
Can you put the comment above the method name?
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. 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)
An issue was opened. Show all issues
```
The outer parens can be removed here.
```

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

An issue was opened. 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)

An issue was opened. 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)
An issue was opened. Show all issues
```
Should have a space after //. This should also be marked 'TODO'
```

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

An issue was opened. 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)
An issue was opened. Show all issues
```
Remove this line.
```
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
An issue was opened. Show all issues
```
Backbone.Events should already be mixed in to any view.
```

Status:: Discarded