Summary

DnD inline images frontend.

Review Request #8147 — Created May 6, 2016 and submitted May 19, 2016, 2:09 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

4db0143...

Reviewers

Groups

reviewboard

People

Description

This change implements the frontend for letting users drag-and-drop images into
their markdown editors. This will upload the image as a user file attachment
and insert the relevant markdown syntax into the editor.

This is based on work by David Kus at https://reviews.reviewboard.org/r/6510/
Compared to that change, I've reworked the UI a bit to avoid the overlapping
drop targets (the review request file attachments drop target is now isolated
to the "Files" section, which appears momentarily if it wasn't already
visible). I've also done a bunch of refactoring of the code to modernize and
clean it up.

At the moment, this adds overlays over the entire text field, and will insert
the markdown text as a new line wherever the cursor is. I'd like to improve
this in the future to make it more obvious where the new entry will end up
(basically moving the cursor during the dragover with inline feedback) but
that's a larger change.

Testing Done


Dragged images into a variety of text fields.
Dragged files to create new file attachments.
Ran unit tests.
Ran js-tests.

Issues

Description	From	Last Updated
Since RB.BaseResource.prototype.defaults is a method that returns an object fo extra data, we'll want to make this a function.	brennie	May 6, 2016, 4:31 p.m.
Can we do: const url = `${SITE_ROOT}api/users/${this.get('username')}/file-attachments/`; or similar?	brennie	May 6, 2016, 4:32 p.m.
you can use { $target } here as shorthand for { $target: $target }	brennie	May 6, 2016, 5:02 p.m.
You can do: const target = new DnDDropTarget({ $target, dropText, callback });	brennie	May 6, 2016, 5:02 p.m.
Why do we need the !important? Can we rework things to not need it?	chipx86	May 15, 2016, 12:39 p.m.
What does the 64px relate to? Can we add a constant?	chipx86	May 15, 2016, 1:38 p.m.
Can we add constants for these colors and border size?	chipx86	May 15, 2016, 4:05 p.m.
Can we use a relative font size (percentage)?	chipx86	May 15, 2016, 4:05 p.m.
Same here.	chipx86	May 15, 2016, 4:05 p.m.
Should use the standard multi-line comment format.	chipx86	May 15, 2016, 1:39 p.m.
Are we limiting this exclusively to images? I can think of other file formats that would be useful in discussions.	chipx86	May 15, 2016, 12:37 p.m.
"append"	chipx86	May 15, 2016, 1:39 p.m.
This is too long.	chipx86	May 15, 2016, 1:39 p.m.
While the new shorthand in ES6 probably has some good uses, I think it's not very future-proof. If anyone ever …	chipx86	May 15, 2016, 4:05 p.m.
One too many "anys," probably.	chipx86	May 15, 2016, 1:39 p.m.
You can do this.el.value += `\n${text}`; here	brennie	May 18, 2016, 4:16 p.m.
Same here.	brennie	May 18, 2016, 4:16 p.m.
(Almost) the same here.	brennie	May 18, 2016, 4:16 p.m.
And here.	brennie	May 18, 2016, 4:16 p.m.
Should these be .jpeg etc? Because I couldn't I upload foojpeg and it would allow it? Or do something like: …	brennie	May 19, 2016, 10:27 a.m.
You can do callback() {} in the object literal.	brennie	May 18, 2016, 4:16 p.m.
Can we put the comparison in parens, to help visually distinguish it better from the => ?	chipx86	May 19, 2016, 10:27 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    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/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Since RB.BaseResource.prototype.defaults is a method that returns an object fo extra data, we'll want to make this a function.

david May 6, 2016, 5:02 p.m.

We can just call it as a function, actually.

Looks like there are several other places with the same issue. I'll fix those up in a separate change.

reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Can we do:

const url = `${SITE_ROOT}api/users/${this.get('username')}/file-attachments/`;


or similar?

reviewboard/static/rb/js/views/dndUploaderView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
you can use { $target } here as shorthand for { $target: $target }
```

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

The issue has been resolved. Show all issues

You can do:

const target = new DnDDropTarget({
    $target,
    dropText,
    callback
});

Commit:

-	de54bea7c3cf00c050dd6a90a15b80b916759af1
+	5cf6e65c16f103e6d26c1d1c2554f4b4084dc363

Diff:

Revision 2 (+663 -110)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
	7 more

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    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/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Change Summary:

Fix defaults.

Commit:

-	5cf6e65c16f103e6d26c1d1c2554f4b4084dc363
+	2d45bf570db45231b2fffa30b1c3ab7ab9427e5b

Diff:

Revision 3 (+665 -110)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
	7 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    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/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Can you upload some screenshots? Would love to see this.

chipx86 May 15, 2016, 3:43 a.m.

A video/animation would also be very cool for this.

david May 15, 2016, 4:43 p.m.

Will make one tomorrow. I do want to tweak the visuals a bit but this change was getting pretty big already.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 3)

The issue has been dropped. Show all issues

Why do we need the !important? Can we rework things to not need it?

david May 15, 2016, 12:39 p.m.

We do need it. The files box is shown in jquery using show() and hide(), which updates the inline style in the DOM. This needs to take precedence over that.

reviewboard/static/rb/css/pages/reviews.less (Diff revision 3)
The issue has been resolved. Show all issues
```
What does the 64px relate to? Can we add a constant?
```
1. david May 15, 2016, 12:39 p.m.
  It's just pulled out of a hat. I'll add a comment.

reviewboard/static/rb/css/ui/dnd-uploader.less (Diff revision 3)

The issue has been resolved. Show all issues

Can we add constants for these colors and border size?

david May 15, 2016, 12:39 p.m.

Constants are useful when things are used more than once. In this case they're not.

chipx86 May 15, 2016, 1:47 p.m.

I've been trying to move all "theme"-y values to constants so it's easier to update as we change styles. When I was doing the 2.5 restyling, it was much easier to work with the constants than to hunt for the values. I ended up adding constants for a lot of things because of it. I'd like to make this easier on my future self.

reviewboard/static/rb/css/ui/dnd-uploader.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we use a relative font size (percentage)?
```
reviewboard/static/rb/css/ui/dnd-uploader.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Same here.
```
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Should use the standard multi-line comment format.
```

reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 3)

The issue has been dropped. Show all issues

Are we limiting this exclusively to images? I can think of other file formats that would be useful in discussions.

david May 15, 2016, 12:39 p.m.

Well yes, because the markdown syntax will turn it into an <img> tag...

Perhaps in the future we'll add the capability to insert some kind of download link instead for non-image types, but I think that's definitely a separate change.

reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
"append"
```
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This is too long.
```

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

The issue has been dropped. Show all issues

While the new shorthand in ES6 probably has some good uses, I think it's not very future-proof. If anyone ever changes one of these variable names down the road for any reason (or in some other case where we use this syntax), we will break.
I don't mind using it for the case where you're defining an object inline and want to give it those properties with those values, but when we're calling into another function (here or in findWhere above, for example), we should be explicit.

david May 15, 2016, 12:39 p.m.

I'm not sure I agree. It's new syntax to learn, and sure, it does require being aware of how it works, but it's no less fragile to search/replace than it would be if it were { $target: $target }, and it's a lot more DRY.

chipx86 May 15, 2016, 1:47 p.m.

Okay. I think this will bite us eventually in a confusing way, when someone mistakes the {} for a [] or something and starts changing variables, but go for it.

chipx86 May 15, 2016, 2:03 p.m.

And to clarify my position, I think these are useful for the case of creating local objects that you know are going to be based on particular variables and have those variables' names. Something like:

function createFoo(a, b, c) {
    return {a, b, c};
}


I just don't like it when calling into external functions/objects. I like being explicit about saying "I'm fulfilling these specific keyword arguments/fields of this function/class, and providing these values," not "these keyword arguments/fields happen to match these variable names, yay magic."

reviewboard/static/rb/js/views/dndUploaderView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
One too many "anys," probably.
```

Commit:

-	2d45bf570db45231b2fffa30b1c3ab7ab9427e5b
+	2dd756ae1e67aa67749c5c52748e6d1278a96e7f

Diff:

Revision 4 (+682 -111)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
	7 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    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/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

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

The issue has been dropped. Show all issues

You can do
this.el.value += `\n${text}`;


here

david May 18, 2016, 4:17 p.m.

I don't think that's really an improvement. Template strings are cool for medium-complexity things but for something simple like this I think it's more readable this way.

reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
Same here.
```
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
(Almost) the same here.
```
reviewboard/static/rb/js/ui/views/textEditorView.es6.js (Diff revision 4)
The issue has been dropped. Show all issues
```
And here.
```

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

The issue has been resolved. Show all issues

Should these be .jpeg etc? Because I couldn't I upload foojpeg and it would allow it?
Or do something like:

return [/*...*/].some(extension => filename.endsWith(`.${extension}`));

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

The issue has been dropped. Show all issues

You can do callback() {} in the object literal.

david May 18, 2016, 4:17 p.m.

As I think I've mentioned before, I like the callback() {} syntax when something looks like a class, but in this case it doesn't.

Looks good. Only one comment, beyond Barret's that were fixed.

reviewboard/static/rb/js/views/dndUploaderView.es6.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can we put the comparison in parens, to help visually distinguish it better from the => ?
```

Commit:

-	2dd756ae1e67aa67749c5c52748e6d1278a96e7f
+	4db01439c0a75769090d6f1afcb18223165fdc6c

Diff:

Revision 5 (+682 -111)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/css/common.less
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/reviews.less
	reviewboard/static/rb/css/ui/dnd-uploader.less
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
	reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
	7 more

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    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/resources/models/tests/userFileAttachmentModelTests.js
    reviewboard/static/rb/css/ui/dnd-uploader.less
    reviewboard/static/rb/js/resources/models/userFileAttachmentModel.es6.js
    reviewboard/static/rb/js/ui/views/textEditorView.es6.js
    reviewboard/static/rb/js/views/dndUploaderView.es6.js
    reviewboard/static/rb/js/pages/views/tests/reviewablePageViewTests.js
    reviewboard/static/rb/js/views/reviewReplyEditorView.js
    reviewboard/templates/reviews/review_request_box.html
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/static/rb/js/ui/views/tests/textEditorViewTests.js
    reviewboard/static/rb/css/common.less
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/views/reviewRequestEditorView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to ucosp/dkus/dnd-inline-images (c85bea4)