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

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

rvaiya
Review Board
master
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?

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

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

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

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

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

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

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

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

  4. Indentation is off here.

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

  6. Add a space after //

  7. What does this comment mean?

  8. Can you add the word "TODO" here?

  9. Can you put the comment above the method name?

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

  11. The outer parens can be removed here.

  12. 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. 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. Should have a space after //. This should also be marked 'TODO'

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

  16. Remove this line.

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

  18. 
      
RV
Review request changed

Status: Discarded

Loading...