[WIP] Front end DnD Editor for per-user file storage
Review Request #5954 — Created June 8, 2014 and discarded
Information | |
---|---|
rvaiya | |
Review Board | |
master | |
Reviewers | |
reviewboard, students | |
Ready for the first iteration of review.
- 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.
Description | From | Last Updated |
---|---|---|
RB.MarkdownFileView? |
|
|
Just a nit - we tend to put the documentation for a function above the function header. It's also a … |
|
|
What is a "file" in this context? Is it something that we'd be able to get a mimetype from? That … |
|
|
No multiple drops? |
|
|
This should use "true" and "false" rather than "1" and "0". |
|
|
We always use brackets around conditional blocks: if (!this.active) { return; } In this specific case, because there's already a … |
|
|
Indentation should be 4 spaces with no tabs. You should check the rest of your changes for proper indentation. |
|
|
It's only really common for web browsers to show .gif, .png, and .jpg/jpeg. I don't see any need to support … |
|
|
This doesn't seem to "append" for codemirror fields. |
|
|
Care to put this above the method instead of inside it? |
|
|
Indentation is off here. |
|
|
I'm not sure what this comment means. Is this supposed to be a to-do item? |
|
|
Add a space after // |
|
|
What does this comment mean? |
|
|
Can you add the word "TODO" here? |
|
|
Can you put the comment above the method name? |
|
|
Should be only one blank line. Please also add a comment for attachFileView |
|
|
The outer parens can be removed here. |
|
|
The var statement should be at the beginning of the function (to make the hoisting clear). This line needs some … |
|
|
We should lower-case file.name before comparing, especially if someone is coming from a case-insensitive filesystem. It might also make sense … |
|
|
Should have a space after //. This should also be marked 'TODO' |
|
|
Should have a space after // (or just get rid of this--I think it's pretty clear that these are event … |
|
|
Remove this line. |
|
|
Backbone.Events should already be mixed in to any view. |
|

-
This is a review from Review Bot. Tool: Pyflakes Processed Files: Ignored Files: reviewboard/static/rb/js/views/markdownEditorView.js
Description: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||
Commit: |
|
||||||||||||
Diff: |
Revision 2 (+104 -2) |

-
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?
-
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 2) 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) 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.
-
Testing Done: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 3 (+169 -3) |

-
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) This should use "true" and "false" rather than "1" and "0".
-
reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 3) 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) 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) 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: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+172 -4) |

-
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/markdownEditorView.js (Diff revision 4) This doesn't seem to "append" for codemirror fields.
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) Care to put this above the method instead of inside it?
-
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) 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) Can you add the word "TODO" here?
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) Can you put the comment above the method name?
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) Should be only one blank line. Please also add a comment for
attachFileView
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) The outer parens can be removed here.
-
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) 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) 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) Should have a space after
//
. This should also be marked 'TODO' -
reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4) 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) Backbone.Events
should already be mixed in to any view.