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 … |
|
|
Can you add screenshots showing this off? |
|
|
Undo this. |
|
|
So unfortunately, doing this requires the element corresponding to the review request editor to be focused (which you've noticed as … |
|
|
This will need a doc comment of the form /** * Handle a paste event. * * Args: * e … |
|
|
We can combine this into: if ($(pasteEvent.target).is('textarea, input') || pasteEvent.clipboarData.files.length === 0) { return; } |
|
|
This should use ===. See mdn for details. |
|
|
Not a super helpful comment. Since we're checking file.type I think it is self explanatory. |
|
|
This can be a single line, e.g. reader.onload = e => this._onPasteFinished(e, file); |
|
|
blank line between these. |
|
|
Lets not handle this case specifically. If we drop this case, the readAsArrayBuffer case should handle it properly. |
|
|
no blank line here. |
|
|
Combine these. |
|
|
Combine these. |
|
|
Remove this line. |
|
|
How about we call this _onFilePasted? Likewise this will need a comment of the form: /** * Handle a file … |
|
|
As I mentioned previously, the current approach requires us to click on the page to focus an element inside this … |
|
|
Missing a blank line. |
|
|
This should use const. We never want to use var in .es6.js files. |
|
|
Here, too. |
|
|
Remove this blank line. |
|
|
This needs a doc comment. Likewise all methods in here. |
|
|
Lets add primary: true to this. |
|
|
Instead of this, we can call _.bindAll(this, '_onUploadClicked'); before the RB.DialogView.prototype.initialite.call(...) and change this to: onClick: this._onUploadClicked, |
|
|
] on next line. |
|
|
Format as: attachment.save({ success: () => document.location.reload(), error: (model, xhr) => console.log($.parseJSON(xhr.responseText)), }); Also, we should probably replace consoel.log with … |
|
|
Blank line between these. |
|
|
How about: this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, }))); |
|
|
Combine these. |
|
|
Unnecessary. |
|
|
Unnecessary. |
|
|
Instead of setting width=50, we should add a class to the <pre> tag and style it with CSS. |
|
|
We might want to render a generic "we could not generate a thumbnail" caption or something. |
|
|
One too many blank lines. |
|
|
Col: 9 Forgotten 'debugger' statement? |
![]() |
|
Col: 37 Missing semicolon. |
![]() |
|
Col: 29 Missing '()' invoking a constructor. |
![]() |
|
Col: 80 Missing semicolon. |
![]() |
|
Too many blank lines. There should only be 2. |
|
|
Too many spaces here. |
|
|
This comment isn't really saying anything that's useful for us down the road. If this is a TODO, it should … |
|
|
Blank line between these. |
|
|
Too many spaces here. |
|
|
This needs a doc comment block. It must also describe each of the attributes using our standard doc style. Notion … |
|
|
No blank lines here. |
|
|
This needs to describe what the function is doing. |
|
|
Indentation is all wrong, and the blank line should be removed. |
|
|
This console.log should go away. Also, blank line between statements and blocks. |
|
|
I can see this biting us in the future accidentally. For instance, I bet you it breaks CodeMirror. This needs … |
|
|
Too many spaces. |
|
|
Should have parens around the whole e => ... to help with readability, given the assignment. |
|
|
Missing a comment doc block. |
|
|
Indentation is all wrong. |
|
|
Missing a doc comment block. |
|
|
This makes it sound like something went wrong, but in reality we just simply can't generate thumbnails for many types … |
|
|
This is too long for the line. |
|
|
Indentation is broken all throughout. |
|
|
This should probably be a <th>, not a <td> with a <label>. |
|
|
The pattern we use elsewhere is to feed in localized text into the template when invoking it, rather than feeding … |
|
|
The <tr> is empty, and the <img> isn't in a valid place. I'm having a hard time imagining how the … |
|
|
This needs to describe what this class is all about. |
|
|
Blank line between these. |
|
|
Blank line between these. |
|
|
The options need to be described in an Option Args section. Grep around for examples. |
|
|
No blank line. |
|
|
No space around the =. Should be options={}. |
|
|
This needs to describe what the function does. |
|
|
No space before (). |
|
|
Space after :. |
|
|
There should only be one blank line at this level. |
|
|
No need to capitalize "dialog." |
|
|
Comment isn't formatted right. |
|
|
This isn't the class name. |
|
|
This isn't safe, for the same reasons as above. There's also no reason to take up memory with a predefined … |
|
|
Blank line between block and statement. |
|
|
Missing , at the end of this. |
|
|
This should go up in the models section. |
|
-
-
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 (...) { // ... } }
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) 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: -
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) This will need a doc comment of the form
/** * Handle a paste event. * * Args: * e (jQuery.Event): * The event that triggered this handler. * */
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) We can combine this into:
if ($(pasteEvent.target).is('textarea, input') || pasteEvent.clipboarData.files.length === 0) { return; }
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) This should use
===
. See mdn for details. -
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) Not a super helpful comment. Since we're checking
file.type
I think it is self explanatory. -
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) This can be a single line, e.g.
reader.onload = e => this._onPasteFinished(e, file);
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) blank line between these.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) Lets not handle this case specifically. If we drop this case, the
readAsArrayBuffer
case should handle it properly. -
-
-
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) 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. */
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) 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()
. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Missing a blank line.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) This should use
const
. We never want to usevar
in.es6.js
files. -
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Remove this blank line.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) This needs a doc comment. Likewise all methods in here.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Lets add
primary: true
to this. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Instead of this, we can call
_.bindAll(this, '_onUploadClicked');
before theRB.DialogView.prototype.initialite.call(...)
and change this to:onClick: this._onUploadClicked,
-
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Format as:
attachment.save({ success: () => document.location.reload(), error: (model, xhr) => console.log($.parseJSON(xhr.responseText)), });
Also, we should probably replace
consoel.log
withalert
. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Blank line between these.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) How about:
this.$el.append($(imageTemplate({ fileContent: this.model.get('fileContent'), fileName: file.name, })));
-
-
-
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) Instead of setting width=50, we should add a class to the
<pre>
tag and style it with CSS. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) We might want to render a generic "we could not generate a thumbnail" caption or something.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 1) One too many blank lines.
Bugs: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 2 (+184 -1) |
Checks run (1 failed, 1 succeeded)
JSHint
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2) Col: 9 Forgotten 'debugger' statement?
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2) Col: 37 Missing semicolon.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 2) Col: 29 Missing '()' invoking a constructor.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 2) Col: 80 Missing semicolon.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+183 -1) |
Checks run (2 succeeded)
-
-
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 3) Too many blank lines. There should only be 2.
-
-
reviewboard/static/rb/css/pages/review-request.less (Diff revision 3) 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. */
-
-
-
reviewboard/static/rb/js/models/uploadPreviewDialogModel.es6.js (Diff revision 3) 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.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) No blank lines here.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) This needs to describe what the function is doing.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) Indentation is all wrong, and the blank line should be removed.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) This
console.log
should go away.Also, blank line between statements and blocks.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) 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.
-
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) Should have parens around the whole
e => ...
to help with readability, given the assignment. -
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) Missing a comment doc block.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) Indentation is all wrong.
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 3) Missing a doc comment block.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) 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."
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) This is too long for the line.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Indentation is broken all throughout.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) This should probably be a
<th>
, not a<td>
with a<label>
. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) 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. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) 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?
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) This needs to describe what this class is all about.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Blank line between these.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Blank line between these.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) The options need to be described in an
Option Args
section. Grep around for examples. -
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) No space around the
=
. Should beoptions={}
. -
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) This needs to describe what the function does.
-
-
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) There should only be one blank line at this level.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) No need to capitalize "dialog."
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Comment isn't formatted right.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) This isn't the class name.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) 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);
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Blank line between block and statement.
-
reviewboard/static/rb/js/views/uploadPreviewDialogView.es6.js (Diff revision 3) Missing
,
at the end of this. -