Drag 'n Drop Inline Images Frontend

Review Request #6510 — Created Oct. 26, 2014 and discarded

dkus
Review Board
master
reviewboard, students

Adding support for dragging and dropping images into the markdown editor. Instead of having upload their image first and then link to that image, users can simply drag-and-drop their image file into the markdown editor. This will upload the image to reviewboard and insert a link to that image in the editor.

These changes focus on the frontend work and include:

  • Extending the DnDUploaderView to support multiple overlays on the page. Multiple DropTargets can be registered with the DnDOverlay instance.
  • Extending the TextEditorView to support dragging and dropping images
  • Adding a resource for UserFileAttachments

Tested in Chrome 38.0.2125.122, Firefox 32.0.2, Safari 8.0, Internet Explorer 11.0.9600

All existing unit tests pass.

  • Added tests for the changes done to the TextEditorView (testing the appendText method and the Drag and Drop functionality).
  • Added tests for serialization and parsing of the UserFileAttachment resource.
  • Added tests for the DnDUploader functionality.

Description From Last Updated

It might be better to use the / / multi line comments here

ML mloyzer

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Variable definitions should all be the first thing in the method.

daviddavid

Can you leave this as-is?

daviddavid

I'd suggest defining a $window as the first thing, and then using that for both. That way it doesn't have …

daviddavid

Wrap to 80 column.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Wrap to 80 columns.

daviddavid

Nit - let's put these in the same order as the function args.

mike_conleymike_conley

Might as well throw bmp, tiff and svg in there too.

mike_conleymike_conley

This can be: return imageExtensions.some(function(extension) { return file.name.match(new RegExp("\\." + extension + "$", "i")); }); That'll work in IE9 and …

mike_conleymike_conley

Nit "and" -> "an"

mike_conleymike_conley

Even though this is supposed to be a very cheap request, synchronous XHR is best to be avoided, since it …

mike_conleymike_conley

We probably need some error handling here in case things go wrong with the upload.

mike_conleymike_conley

Where are these magic 6's coming from?

mike_conleymike_conley

Ah - is there were the magic 6 comes from? Can we apply an attribute or class on the element …

mike_conleymike_conley

Might be good to get this stuff in CSS instead - perhaps a class for the window overlay. Or a …

mike_conleymike_conley

This seems like it wants to be a model but doesn't want to adhere to model conventions. Models work with …

chipx86chipx86

This doesn't buy us anything that new already gives us. It's actually nicer to pass in the arguments as a …

chipx86chipx86

Blank line between these.

chipx86chipx86

And here.

chipx86chipx86

Two blank lines.

chipx86chipx86

Blank line between these.

chipx86chipx86

Most of these can go inline below, saving some lines of code.

chipx86chipx86

This can break down the road if styles are changed. Instead, use outerWidth() and outerHeight(), which account for the border.

chipx86chipx86

This should be a single .css({ ... }) statement.

chipx86chipx86

The var should go at the top of the file.

chipx86chipx86

Same comments here as above.

chipx86chipx86

No blank line here.

chipx86chipx86

Two blank lines.

chipx86chipx86

Not every page will need this. Is it not sufficient to create an instance in ReviewRequestEditorView, or any other pages …

chipx86chipx86
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/fileAttachmentUserModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
  2. 
      
dkus
chipx86
  1. Is this still valid, or has it been replaced by /r/6454/?

    1. This is indeed still valid. These changes cover the frontend work, the other review request is for the backend.

  2. 
      
dkus
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/models/userFileModel.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/models/userFileModel.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/commentDialogView.js
        reviewboard/static/rb/js/views/markdownEditorView.js
    
    
  2. 
      
ML
  1. 
      
  2. It might be better to use the / / multi line comments here

  3. 
      
dkus
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/models/userFileModel.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/pages/views/pageView.js
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/models/userFileModel.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/pages/views/pageView.js
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/resources/models/apiTokenModel.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
dkus
dkus
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. 
      
  2. Wrap to 80 columns.

  3. Wrap to 80 columns.

  4. Variable definitions should all be the first thing in the method.

  5. Can you leave this as-is?

  6. I'd suggest defining a $window as the first thing, and then using that for both. That way it doesn't have to run the jquery selector twice.

  7. Wrap to 80 columns.

  8. 
      
dkus
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
dkus
mike_conley
  1. 
      
  2. reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 5)
     
     
     
     

    Nit - let's put these in the same order as the function args.

  3. Might as well throw bmp, tiff and svg in there too.

  4. reviewboard/static/rb/js/ui/views/textEditorView.js (Diff revision 5)
     
     
     
     
     
     
     

    This can be:

    return imageExtensions.some(function(extension) { return file.name.match(new RegExp("\\." + extension + "$", "i")); });

    That'll work in IE9 and up.

  5. Nit "and" -> "an"

  6. Even though this is supposed to be a very cheap request, synchronous XHR is best to be avoided, since it 100% blocks JS execution. If there are network problems (say, our DNS server is flaking out on us), this can absolutely lock up everything in the user's tab (or their entire browser).

    Is there a way of doing this asynchronously, and passing in a callback to do the upload of the whole file? Also, what happens if that first save fails?

  7. We probably need some error handling here in case things go wrong with the upload.

    1. What would you recommend happen if things go wrong? Also, it can fail at 2 places (when doing the first POST and then after when updating with the file), should we handle both errors?

    2. Good question. Our error reporting isn't very consistent - at times, we show an error that can be unfolded from the "Loading" spinner when we're updating a review request. Other times, we pop up an alert (ugh). Othertimes, there's inline error reporting (like when you try to set a reviewer on a review request to a username that does not exist).

      Do we show any kind of progress or spinner while the upload is occurring? Ideally, the error would be located in or around there.

    3. We don't show any progress / spinner while uploading right now. It's something that we decided to cut out so that I could have something finished before the pencils down date.

    4. Ah, right. In that case, we should at least log it to the error console, and then follow up bugs for more robust error reporting.

    5. Alright. Should I just use console.error()? I don't see that being used anywhere in the code base.

  8. Where are these magic 6's coming from?

  9. Ah - is there were the magic 6 comes from?

    Can we apply an attribute or class on the element instead, and have a selector to apply the border? Putting non-positional styling in JS should be avoided if possible.

  10. Might be good to get this stuff in CSS instead - perhaps a class for the window overlay. Or a separate class for the text input overlays.

  11. 
      
dkus
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
dkus
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
chipx86
  1. Looking good. Got some things to fix up that are largely stylistic or JavaScript/jQuery/Backbone-isms that will help reduce code and logic.

  2. reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    This seems like it wants to be a model but doesn't want to adhere to model conventions.

    Models work with attributes instead of direct properties (like target). It also uses defaults for any default options.

    You probably want to fill defaults with default values for target, dropAction, context, and active. You can then remove the initialize call.

    Consumers will then need to access this data using get().

    This is also missing documentation.

  3. reviewboard/static/rb/js/models/dndDropTarget.js (Diff revision 7)
     
     
     
     
     
     
     
     

    This doesn't buy us anything that new already gives us. It's actually nicer to pass in the arguments as a dictionary instead of positional, since it's more self-documenting.

  4. Blank line between these.

  5. Two blank lines.

  6. Blank line between these.

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

    Most of these can go inline below, saving some lines of code.

  8. This can break down the road if styles are changed. Instead, use outerWidth() and outerHeight(), which account for the border.

    1. This won't really work for my case. It's not the target that has the borders, it's the element who's height/width I'm setting. I would need to do something like setOuterWidth() and setOuterHeight() (I don't think those exist). Just setting the height of the element doesn't include it's borders, so with the borders it exceeds the height of the target.

    2. Ahh. What you can do then is:

      var width = target.width() + this.$el.getExtents('b', 'lr'),
          height = target.height() + this.$el.getExtents('b', 'tb');
      

      That will get the total border size for the proper dimensions on this.$el add that to the target width/height.

    3. Works fine in Firefox but doesn't seem to work in Chrome (returning 0 instead of 6).

  9. This should be a single .css({ ... }) statement.

  10. The var should go at the top of the file.

  11. Same comments here as above.

  12. No blank line here.

  13. Two blank lines.

  14. Not every page will need this. Is it not sufficient to create an instance in ReviewRequestEditorView, or any other pages that need this?

    1. What if I create it in the ReviewablePageView?

    2. That works fine too.

  15. 
      
dkus
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
dkus
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/staticbundles.py
    
    Ignored Files:
        reviewboard/static/rb/js/models/dndDropTarget.js
        reviewboard/static/rb/css/ui/dnd-uploader.less
        reviewboard/static/rb/js/views/tests/dndUploaderViewTests.js
        reviewboard/static/rb/js/resources/models/userFileAttachmentModel.js
        reviewboard/static/rb/js/views/dndUploaderView.js
        reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
        reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
        reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
        reviewboard/static/rb/css/common.less
        reviewboard/static/rb/js/ui/views/textEditorView.js
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. 
      
dkus
Review request changed

Status: Discarded

Change Summary:

Discarded in favor of /r/8147/

Loading...