Allow pasting of images to upload
Review Request #10078 — Created July 10, 2018 and updated
The user can now attach an image to a review request by pasting. This
action opens a dialog box showing where the user is shown the image
being uploaded and is able to add a caption for the image, or cancel the
upload.
Description | From | Last Updated |
---|---|---|
So I stepped thorugh this and was able to figure out what the cause. THe way that browsers load images … |
brennie | |
Can you add screenshots showing this off? |
chipx86 | |
Undo this. |
brennie | |
So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as … |
brennie | |
This will need a doc comment of the form /** * Handle a paste event. * * Args: * e … |
brennie | |
We can combine this into: if ($(pasteEvent.target).is('textarea, input') || pasteEvent.clipboarData.files.length === 0) { return; } |
brennie | |
This should use ===. See mdn for details. |
brennie | |
Not a super helpful comment. Since we're checking file.type I think it is self explanatory. |
brennie | |
This can be a single line, e.g. reader.onload = e => this._onPasteFinished(e, file); |
brennie | |
blank line between these. |
brennie | |
Lets not handle this case specifically. If we drop this case, the readAsArrayBuffer case should handle it properly. |
brennie | |
no blank line here. |
brennie | |
Combine these. |
brennie | |
Combine these. |
brennie | |
Remove this line. |
brennie | |
How about we call this _onFilePasted? Likewise this will need a comment of the form: /** * Handle a file … |
brennie | |
As I mentioned previously, the current approach requires us to click on the page to focus an element inside this … |
brennie | |
Missing a blank line. |
brennie | |
This should use const. We never want to use var in .es6.js files. |
brennie | |
Here, too. |
brennie | |
Remove this blank line. |
brennie | |
This needs a doc comment. Likewise all methods in here. |
brennie | |
Lets add primary: true to this. |
brennie | |
Instead of this, we can call _.bindAll(this, '_onUploadClicked'); before the RB.DialogView.prototype.initialite.call(...) and change this to: onClick: this._onUploadClicked, |
brennie | |
] on next line. |
brennie | |
Format as: attachment.save({ success: () => document.location.reload(), error: (model, xhr) => console.log($.parseJSON(xhr.responseText)), }); Also, we should probably replace consoel.log with … |
brennie | |
Blank line between these. |
brennie | |
How about: this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, }))); |
brennie | |
Combine these. |
brennie | |
Unnecessary. |
brennie | |
Unnecessary. |
brennie | |
Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS. |
brennie | |
We might want to render a generic "we could not generate a thumbnail" caption or something. |
brennie | |
One too many blank lines. |
brennie | |
Col: 9 Forgotten 'debugger' statement? |
reviewbot | |
Col: 37 Missing semicolon. |
reviewbot | |
Col: 29 Missing '()' invoking a constructor. |
reviewbot | |
Col: 80 Missing semicolon. |
reviewbot | |
Too many blank lines. There should only be 2. |
chipx86 | |
Too many spaces here. |
chipx86 | |
This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should … |
chipx86 | |
Blank line between these. |
chipx86 | |
Too many spaces here. |
chipx86 | |
This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion … |
chipx86 | |
No blank lines here. |
chipx86 | |
This needs to describe what the function is doing. |
chipx86 | |
Indentation is all wrong, and the blank line should be removed. |
chipx86 | |
This console.log should go away. Also, blank line between statements and blocks. |
chipx86 | |
I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror. This needs … |
chipx86 | |
Too many spaces. |
chipx86 | |
Should have parens around the whole e => ... to help with readability, given the assignment. |
chipx86 | |
Missing a comment doc block. |
chipx86 | |
Indentation is all wrong. |
chipx86 | |
Missing a doc comment block. |
chipx86 | |
This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types … |
chipx86 | |
This is too long for the line. |
chipx86 | |
Indentation is broken all throughout. |
chipx86 | |
This should probably be a <th>, not a <td> with a <label>. |
chipx86 | |
The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding … |
chipx86 | |
The <tr> is empty, and the <img> isn't in a valid place. I'm having a hard time imagining how the … |
chipx86 | |
This needs to describe what this class is all about. |
chipx86 | |
Blank line between these. |
chipx86 | |
Blank line between these. |
chipx86 | |
The options need to be described in an Option Args section. Grep around for examples. |
chipx86 | |
No blank line. |
chipx86 | |
No space around the =. Should be options={}. |
chipx86 | |
This needs to describe what the function does. |
chipx86 | |
No space before (). |
chipx86 | |
Space after :. |
chipx86 | |
There should only be one blank line at this level. |
chipx86 | |
No need to capitalize "dialog." |
chipx86 | |
Comment isn't formatted right. |
chipx86 | |
This isn't the class name. |
chipx86 | |
This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined … |
chipx86 | |
Blank line between block and statement. |
chipx86 | |
Missing , at the end of this. |
chipx86 | |
This should go up in the models section. |
chipx86 |
- People:
-
-
So I stepped thorugh this and was able to figure out what the cause. THe way that browsers load images is asynchronous. Just because we have the
data:
URI doesn't mean that Firefox/Chrome etc. know the dimensions of the image. It only knows those dimensions once we create the<img>
and set thesrc
attribtue. Then it starts parsing and loading the image.However, by then, we've already finished
render()
and the parent class will have resized itself with the original size of the image (which is 0 px wide and 0 px high). Once we finish rendering, the main thread is unblocked and the browser will start loading the image. Once the image has finished loading, it renders, but we've already resized so it looks wonkyThe following is a solution that preloads the image source in render:
render() { const file = this.model.get('file'); if (file.type.startsWith('image/')) { const uri = this.model.get('fileContent'); const image = new Image(); image.onload = () => this.$el.append($(imageTemplate({ fileContent: uri, fileName: file.name, }))); image.src = uri; } else if (...) { // ... } }
-
-
So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as you have to click in the page to paste a file).
Instead of doing this, check out my comment in
render
for an alternate suggestion: -
This will need a doc comment of the form
/** * Handle a paste event. * * Args: * e (jQuery.Event): * The event that triggered this handler. * */
-
We can combine this into:
if ($(pasteEvent.target).is('textarea, input') || pasteEvent.clipboarData.files.length === 0) { return; }
-
-
-
-
-
Lets not handle this case specifically. If we drop this case, the
readAsArrayBuffer
case should handle it properly. -
-
-
-
-
How about we call this
_onFilePasted
?
Likewise this will need a comment of the form:/** * Handle a file being pasted. * * Args: * readEvent (Event): * The event from the `FileReader`. * * file (File): * The file that was pasted. */
-
As I mentioned previously, the current approach requires us to click on the page to focus an element inside this view before we can paste. However, we can do the following in
render
/remove
$(window).on('paste', this._onPaste):
We'll also have to add a
remove()
function:remove() { Backbone.View.prototype.remove.call(this); $(window).off('paste', this._onPaste); }
To get around this-binding, we'll want to add
'_onPaste'
to the list of functions in_.bindAll()
that is called ininitialize()
. -
-
-
-
-
-
-
Instead of this, we can call
_.bindAll(this, '_onUploadClicked');
before theRB.DialogView.prototype.initialite.call(...)
and change this to:onClick: this._onUploadClicked,
-
-
Format as:
attachment.save({ success: () => document.location.reload(), error: (model, xhr) => console.log($.parseJSON(xhr.responseText)), });
Also, we should probably replace
consoel.log
withalert
. -
-
How about:
this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, })));
-
-
-
-
-
-
- Bugs:
-
- Commit:
cec4f08d6bd7f9caf5179fa4ab3c519c62e9f34a8141b49a08d17f9b9c3c7f66482bafb05f1fc9cb- Diff:
Revision 2 (+184 -1)
- Commit:
-
8141b49a08d17f9b9c3c7f66482bafb05f1fc9cbddb087d6aae4c2e5d670918eb0f69bce46162e7d
- Diff:
-
Revision 3 (+183 -1)
Checks run (2 succeeded)
-
-
-
-
-
This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should be prefixe with "TODO:". However, if this is something that should be done, it should be done now.
Comments, btw, should be in sentence casing, ending with a period, and should use
/* .. */
style. So:/* TOD: Use classes for these. */
-
-
-
This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion has a guide on this, and you can grep around for newer classes following the style.
-
-
-
-
-
I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror.
This needs extremely thorough testing, and it needs a comment block explaining why we're testing this and the implications.
-
-
-
-
-
-
This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types of files in the browser (text files, PDFs, etc.). So we should be more clear:
"Thumbnail previews can only be generated for image files. The thumbnail will be generated after upload instead."
-
-
-
-
The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding in variables as above. Doing it the way you have it now will break the template if any of the localized text returned from
gettext
isn't HTML-safe.This should be
<%- captionLabel %>
and that variable should be passed in when invoking below. -
The
<tr>
is empty, and the<img>
isn't in a valid place. I'm having a hard time imagining how the browser is rendering this table.Maybe it shouldn't even be a table?
-
-
-
-
-
-
-
-
-
-
-
-
-
-
This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined string for a rare case like this. Instead, do:
$('<p>') .text(gettext('.....')) .appendTo(this.$el);
-
-
-