Upload images in admin with drag and drop #1553
Conversation
mtomov
changed the title
mtomov
changed the title
|
Hello, This one is ready for official review. I've cleaned up the javascript using Backbone, and have placed the supporting bits (css, js, translations) to their relevant places. This is first time I'm using Backbone and I'm quite pleasantly surprised how it put a nice structure overall. The events mapping is very clean, and the view and its model work very well together to provide for live data binding. You might've noticed that I'm using the underscore's templating function In terms of design - current one is a bit plain. An alternative is to make use of Bootstrap's Cards. So, either multiple images per row, or again a single image per row, but wrapped in a round border. Thank you! |
|
@mtomov thanks! I will have a look later. |
| <p id="formdata">XHR2's FormData is not supported</p> | ||
| <p id="progress">XHR2's upload progress isn't supported</p> | ||
|
|
||
| <script type="text/template" id="upload-progress-tmpl"> |
There was a problem hiding this comment.
Elsewhere we've used handlebars templates which can be thrown in spree/backend/templates which are precompiled and made available to the admin JS.
Edit: Sorry, I see you left a comment noting this.
I would prefer this used handlebars. Both for consistency and because I think it is better here.
There was a problem hiding this comment.
@mtomov this is very cool 🎉 and I left some minor feedback.
I understand your desire to avoid the handlebars template structure over just using a quick underscore template, but this component seems
- Just big enough to be noticeable in this file.
- Something very generic that we might want to use elsewhere.
| }, | ||
|
|
||
| onDragOver: function() { | ||
| this.el.className = 'hover'; |
There was a problem hiding this comment.
Maybe something other than .hover , since the action here is .drag-over.
There was a problem hiding this comment.
Right so. I've gone for that dragClass: 'with-images'
| }, | ||
|
|
||
| onDragLeave: function() { | ||
| this.el.className = ''; |
There was a problem hiding this comment.
If we use jquery's addClass and removeClass we can be more descriptive of what's happening and avoid crushing unrelated classnames.
There was a problem hiding this comment.
Indeed so. Used the native's el.classList.remove/add
|
|
||
| onDragOver: function() { | ||
| this.el.className = 'hover'; | ||
| return false; |
There was a problem hiding this comment.
We might want to be more specific that return false, here we need to e.preventDefault() to allow dropping a file, but we don't need to stopPropagation of the event.
There was a problem hiding this comment.
Good observation. Corrected to that
|
|
||
| onDragLeave: function() { | ||
| this.el.className = ''; | ||
| return false; |
There was a problem hiding this comment.
We don't need to stop or prevent this event.
There was a problem hiding this comment.
Thanks for looking in those details. Corrected now.
| this.el.className = ''; | ||
| e.preventDefault(); | ||
|
|
||
| for (var i = 0; i < e.originalEvent.dataTransfer.files.length; i++) { |
There was a problem hiding this comment.
We could use one of our helper libraries for iterating.
_.each(e.originalEvent.dataTransfer.files, this.upload, this);There was a problem hiding this comment.
did it as per your example - all excellent working. Thanks!
| } | ||
| }, | ||
|
|
||
| tests: { |
There was a problem hiding this comment.
I'm not sure if we need the browser support checking, since these APIs exist on all modern browsers including Microsoft Edge and we're not doing anything in the event of failure, just showing some words.
If we did want the support checking, maybe a custom build of Modernizr would be less application code to support? cc @jhawthorn
There was a problem hiding this comment.
I'm not in favour of adding more libs. The code for those random pieces of text is ~20 links. As you see fit..
| data: formData, | ||
| processData: false, // tell jQuery not to process the data | ||
| contentType: false, // tell jQuery not to set contentType | ||
| xhr: function () { |
There was a problem hiding this comment.
Adding progress to ajax is cool, we may want to make this generic to all Spree.ajax in the future. Make it a callback and trigger an event.
There was a problem hiding this comment.
I think it's too early at this point for anything such. As far as I understand, one only gets progress for large user submitted requests, i.e. files. For regular post requests when the server is just slow .. that's a different story that would likely need server support.
| @@ -0,0 +1,22 @@ | |||
| <tr id="<%= spree_dom_id image %>" data-hook="images_row" class="<%= cycle('odd', 'even')%>"> | |||
There was a problem hiding this comment.
As we add more long lived pages cycle('odd', 'even') is really gonna show it's limitations, as new uploads don't get striped.
- image is automatically assigned the master variant - use latest browsers to make best use of it
.. after successful upload
On error - display error message below uploading row
- Place a permanent Browse button to allow users to also upload images
without drag and drop. Also allow for selecting multiple
- also, display error messages if any from the server
- use underscore's template function, but replace interpolation tags
from the default <% %> to {{ }}
- use progressModel and progressView to manage the progress div row
* By not updating the full progress partial, but only the progress value
- fix minor js omissions
|
Thanks @Sinetheta and John for the elaborate look. I've indeed corrected all points that you mentioned in 31632505f4e3ced01618717a51d5be4b4401fa0f - switched to Handlebars, etc .. Furthermore:
|
* With this, the full class is modeled via backbone classes
- extract CSS out in product.css - extract customer facing text to en.yml - correct minor errors on the create.js - move image preview to the right of the progress bar
- Use Handlebars instead of underscore for templating - User underscore's `_.each` to loop through files - Use a better class for dragged over state
- Update the Handlebar's custom #t helper to handle
string keys with dots.
Source: http://stackoverflow.com/a/22129960/4405214
|
As I can see when I try to upload large image around 20 MB, I see the progress bar show 100% but the loader still running, and when I check it with the server logs it looks like paperclip is still trying to create that file. Could you please let me know is this working as expected as it implemented? |
I wanted to see how much effort it would be to do a modern drag and drop image upload (#1518), and that pretty much does it. Code is
notideal, but wanted to post it here if somebody wants to try it out, have any comments, etc ..In summary:
100 lines(grew to ~ 200 with Backbone), even non-coffee-script (as I started with a non coffee example, but can convert it around).javascriptresponse from the server, which appends a row in the images table .. not sure if it's an accepted practiceAnd some pictures:
or in boxes: