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

Review Request #5954 — Created June 8, 2014 and discarded

Information

Review Board
master

Reviewers

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?

mike_conleymike_conley

Just a nit - we tend to put the documentation for a function above the function header. It's also a …

mike_conleymike_conley

What is a "file" in this context? Is it something that we'd be able to get a mimetype from? That …

mike_conleymike_conley

No multiple drops?

mike_conleymike_conley

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

daviddavid

We always use brackets around conditional blocks: if (!this.active) { return; } In this specific case, because there's already a …

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Indentation is off here.

daviddavid

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

daviddavid

Add a space after //

daviddavid

What does this comment mean?

daviddavid

Can you add the word "TODO" here?

daviddavid

Can you put the comment above the method name?

daviddavid

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

daviddavid

The outer parens can be removed here.

daviddavid

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

daviddavid

We should lower-case file.name before comparing, especially if someone is coming from a case-insensitive filesystem. It might also make sense …

daviddavid

Should have a space after //. This should also be marked 'TODO'

daviddavid

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

daviddavid

Remove this line.

daviddavid

Backbone.Events should already be mixed in to any view.

daviddavid
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
      Ignored Files:
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
  2. 
      
RV
RV
RV
reviewbot
  1. 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
    
    
  2. 
      
mike_conley
  1. 
      
  2. I'm curious, why the switch to the container over the body?

    1. 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').

  3. Show all issues

    RB.MarkdownFileView?

    1. 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".

  4. 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.

  5. 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.

    1. 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.

    2. 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.

  6. Show all issues

    No multiple drops?

    1. Good point, I will fix this.

  7. 
      
RV
RV
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/js/views/dndUploaderView.js (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

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

  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 &&
        ...
    
  4. 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.

  5. 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.

  6. 
      
RV
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
     
     
     
     
     
     
     
     
    Show all issues

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

  3. reviewboard/static/rb/js/views/markdownEditorView.js (Diff revision 4)
     
     
     
     
     
    Show all issues

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

  4. Show all issues

    Indentation is off here.

  5. Show all issues

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

  6. Show all issues

    Add a space after //

  7. Show all issues

    What does this comment mean?

  8. Show all issues

    Can you add the word "TODO" here?

  9. Show all issues

    Can you put the comment above the method name?

  10. Show all issues

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

  11. Show all issues

    The outer parens can be removed here.

  12. 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++) {
    
  13. 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.

  14. Show all issues

    Should have a space after //. This should also be marked 'TODO'

  15. 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.

  16. Show all issues

    Remove this line.

  17. Show all issues

    Backbone.Events should already be mixed in to any view.

  18. 
      
RV
Review request changed
Status:
Discarded