Fixes #1571 - handle multiple image drag and drop events #1634

cch5ng · 2017-07-04T02:06:57Z

fix: #1571; add handler for multiple drag and drop events; update some styling

some history in #1616 also #1593

for the record, I'm not too crazy about this new functionality, after the first image is added to the input field, the user can do one of the following:

drag and drop another image
click the top to select another image
click the bottom to remove the image

my preference would be to limit the user options to either add the image (one state) or remove the image (another state) and not mix it all up. but it is what it is...

update (070617) ps

fwiw this is the list of test case scenarios I came up with; some may be redundant with functional tests. could not figure out at this time how to automate tests for file drag and drop.

X-pass

1 click to add image
2 click to remove image
=> image should be removed

X-pass

1 click to add image

http://localhost:5000/uploads/2017/6/060c17f0-75a9-4bf9-9025-6a5bb04c0908-thumb.jpg

2 click to replace image

http://localhost:5000/uploads/2017/6/db48d0d3-837d-4dd0-8d7b-8d77c0144da5-thumb.jpg

=> image should be updated
=> text area should be updated
3 click to remove image
=> image should be removed

X-pass

1 drag to add image

http://localhost:5000/uploads/2017/6/7c40b638-90cf-4191-87e6-5d7fff288beb-thumb.jpg

=> text area should be updated
2 drag to replace image

http://localhost:5000/uploads/2017/6/fa0d37d9-57ca-41f0-8075-351aac4e2da6-thumb.jpg

=> file input should be updated
=> text area should be updated
3 click to remove image
=> image should be removed

img1
http://localhost:5000/uploads/2017/6/513cb759-c08a-406f-9046-155a2f0e9b28-thumb.jpg

img2
http://localhost:5000/uploads/2017/6/b884ff32-a5d9-4852-a126-3d3d86c1a23b-thumb.jpg

X-pass

1 click to add image

http://localhost:5000/uploads/2017/6/5c1daecb-84ac-46d0-8b22-8cdb787720a8-thumb.jpg

=> text area should be updated
2 drag to replace image

http://localhost:5000/uploads/2017/6/1e28e2d9-b446-45d2-8efd-00fc3089d409-thumb.jpg

=> file input should be updated
=> text area should be updated
3 click to remove image
=> image should be removed

img1
http://localhost:5000/uploads/2017/6/707d338d-8015-48de-8f1e-a47bfea8baec-thumb.jpg

img2
http://localhost:5000/uploads/2017/6/3290d9c8-df3b-4a40-82d9-f5c2327305d1-thumb.jpg

X-pass

1 drag to add image

http://localhost:5000/uploads/2017/6/c5e1098e-aaff-4e22-80b2-ab6dd093df4c-thumb.jpg

=> text area should be updated
2 click to replace image

http://localhost:5000/uploads/2017/6/35d21095-77b1-45a2-acd7-3948263fc67c-thumb.jpg

=> file input should be updated
=> text area should be updated
3 click to remove image
=> image should be removed

img1
http://localhost:5000/uploads/2017/6/7b804914-badf-43ae-a38f-6ce8cf8e945e-thumb.jpg

img2
http://localhost:5000/uploads/2017/6/788fef15-317b-43ab-8e21-01cbe934a55b-thumb.jpg

cch5ng · 2017-07-04T02:14:57Z

@miketaylr r? (should I continue to get JS reviews from you or should I sometimes ask Karl?)

(update) checking notes, forgot there was a dependency on #1306. should I close this until then? also reminder that #1306 is now unblocked.

miketaylr · 2017-07-06T15:13:04Z

@miketaylr R? (should I continue to get JS reviews from you or should I sometimes ask Karl?)

I'm happy to review, but @zoepage and @karlcow are also good candidates (dunno if Karl will self-identify as a JS reviewer :p)

miketaylr · 2017-07-06T15:15:53Z

(update) checking notes, forgot there was a dependency on #1306. should I close this until then? also reminder that #1306 is now unblocked.

Nah, let's not close right now. I'll work on #1306 today/tomorrow, then let's come back to this. Hopefully I don't create too many merge conflicts. :)

cch5ng · 2017-07-06T16:55:34Z

in another issue I've worked on, there was some regex to remove the old image text url from the text area.

https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L460

not sure if it's better to use that or to keep the way I'm doing the url text replacement.

miketaylr · 2017-07-14T20:20:46Z

OK, #1306 has landed. Can you re-base and push to this branch again @cch5ng? There's no conflicts, but it will make future reviews/iterations a bit nicer.

webcompat/static/css/development/components/upload-form.css

@@ -92,6 +94,9 @@
  text-decoration: underline;
  transform: translateY(0);
  transition: transform .2s linear 0s;
+
+  /* workaround for mult dnd */


webcompat/static/js/lib/bugform.js

@@ -155,6 +156,18 @@ function BugForm() {
    img.src = dataURI;
  };

+  // Event handler for image file drop; purpose to detect multiple drops
+  this.handleImageDrop = function() {
+    var imageFilled = false;


webcompat/static/js/lib/bugform.js

@@ -522,8 +542,13 @@ function BugForm() {
      img_url,
      ")"
    ].join("");
+    var delimeter = "[![Screenshot Description](";


webcompat/static/js/lib/bugform.js

+    }
+    if (imageFilled) {
+      // if 2nd drag and drop make wrapper hidden to make remove button available
+      $(".wc-UploadForm-wrapper").addClass("is-hidden");


miketaylr · 2017-07-14T20:46:46Z

in another issue I've worked on, there was some regex to remove the old image text url from the text area.

https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L466-L468 is actually dead code now, I forgot to remove it in #1306. If you can make a commit to fix that in this PR that's cool, or otherwise we can open a new issue.

cch5ng · 2017-07-14T22:51:22Z

@miketaylr this is just a high level response to your questions. I will respond inline to your comments afterward. basically I feel this is a bit of a hack solution to what seems more like a new feature than a bug.

this is the cause of why currently the 2nd drag and drop event replaces the browser url with the 2nd file url:
#1571 (comment)

so the solution I turned in was to prevent ever hiding the input field for image file. but this causes a bunch of breakage of existing logic for that input's state. documented:
#1593 (comment)
#1634 (comment)

looking at bugform.js > showRemoveUpload() around current lines
https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L450
(and about 10 lines down), it looks like the previous author only intended that input field to have 2 states

filled and you can remove the file with the button/link or
empty and you can click/drag 'n drop to add a file

that is why my changes are a hack and make the resulting code difficult to understand. I still believe the cleaner solution would have been more along the lines of Dennis' suggestion of only preventing the 2nd drag and drop attempt from updating the browser url (requiring the user to click Remove before another drag 'n drop can occur): #1571 (comment)
(which I investigated and needed a little more logic but requires little code to resolve and is much clearer to maintain; this is available on a separate local branch)

cch5ng · 2017-07-14T23:14:19Z

@miketaylr

https://github.com/webcompat/webcompat.com/blob/master/webcompat/static/js/lib/bugform.js#L466-L468 is actually dead code now, I forgot to remove it in #1306. If you can make a commit to fix that in this PR that's cool, or otherwise we can open a new issue.

ok, I will add that as a todo here

cch5ng · 2017-07-15T02:25:05Z

@miketaylr I think I made all of your change requests and fixed a couple issues after the rebase. let me know if I missed anything. travis tests running currently (will check if they pass a little later). thanks.

miketaylr

I wonder if some code got lost, or not pushed. I don't see how we're handling the drop event, unless I'm misunderstanding something.

webcompat/static/js/lib/bugform.js

-        this.stepsToReproduceField.val(function(idx, value) {
-          return value.replace(/\[!\[[^\]]+\]\([^\)]+\)\]\([^\)]+\)$/, "");
-        });
+        // this.stepsToReproduceField.val(function(idx, value) {


webcompat/static/js/lib/bugform.js

@@ -485,7 +492,7 @@ function BugForm() {
      // grab the data URI from background-image property, since it's already
      // in the DOM: 'url("data:image/....")'
      this.previewEl.get(0).style.backgroundImage.slice(5, -2),
-      function(response) {
+      _.bind(function(response) {


webcompat/static/css/development/components/upload-form.css

@@ -92,6 +94,9 @@
  text-decoration: underline;
  transform: translateY(0);
  transition: transform .2s linear 0s;
+
+  /* workaround for mult dnd */


miketaylr · 2017-07-18T20:07:52Z

I wonder if some code got lost, or not pushed.

fix: #1571; update to use this.hasImage from #1603 from the commit message makes me think this is likely... I don't see hasImage getting used in this changeset at all.

cch5ng · 2017-07-18T22:29:42Z

re: comment below, that whole function was removed in the latest commit (squashed). I think it was only really useful for an alternative solution to prevent 2nd drag/drop event from changing the browser url. thx. I never pushed that alternative solution to github though.

…

On Tue, Jul 18, 2017 at 3:02 PM, Mike Taylor ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In webcompat/static/js/lib/bugform.js <#1634 (comment)> : > @@ -155,6 +156,18 @@ function BugForm() { img.src = dataURI; }; + // Event handler for image file drop; purpose to detect multiple drops + this.handleImageDrop = function() { + var imageFilled = false; + if ($(".wc-UploadForm-label.is-hidden").length) { + imageFilled = true; + } + if (imageFilled) { + // if 2nd drag and drop make wrapper hidden to make remove button available + $(".wc-UploadForm-wrapper").addClass("is-hidden"); Hm, I'll have to think this over. It feels a bit too accidental to hide a different DOM element to get this to work. I still think we should be listening for drop or dropend events and handling things from there, if we want to do this in a more robust way. But maybe I'm missing something. Will think more about this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1634 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEiP6KW6gJZ1j_2qD-YM6QF32jFnKeU_ks5sPStogaJpZM4OMz4l> .

also update after #1306 fix: #1571; add handler for multiple drag and drop events; update some styling fix: #1571; update to use this.hasImage from #1603 fix: #1571; clean up addImageURL(); add _.bind() wrapper in maybeUploadImage() to fix error for this.addImageURL() call fix: #1571; clean up code added for #1574 fix: #1571; remove obsolete handler, handleImageDrop(); for some reason after #1603 this breaks the intended functionality cleanup: #1571; remove old logic to update text area doc: #1571; update comment for dnd fix

cch5ng · 2017-07-19T15:07:55Z

@miketaylr I think I've made all requested changes on this branch

miketaylr · 2017-07-19T17:38:51Z

OK, this doesn't address my comments at #1634 (comment), but it does work. So let's merge it for now and I'll file a follow up issue.

Also, I can handle this right now, but in the future for a single commit, can you clean up the commit message?

The following has a ton of irrelevant info for the changeset that is landing:

 fix: #1571; update js and styles to handle multiple drag and drop events
cch5ng committed 16 days ago

also update after #1306

fix: #1571; add handler for multiple drag and drop events; update some styling

fix: #1571; update to use this.hasImage from #1603

fix: #1571; clean up addImageURL(); add _.bind() wrapper in maybeUploadImage() to fix error for this.addImageURL() call

fix: #1571; clean up code added for #1574

fix: #1571; remove obsolete handler, handleImageDrop(); for some reason after #1603 this breaks the intended functionality

cleanup: #1571; remove old logic to update text area

doc: #1571; update comment for dnd fix

Just something like Issue #1571. Handle multiple images being dropped into upload area (or similar). You can also break things up into different commits if they need more explanation, or the single commit is doing too much. Thanks!

cch5ng · 2017-07-19T20:48:02Z

@miketaylr

Also, I can handle this right now, but in the future for a single commit, can you clean up the commit message?

thanks for the fix/merge. yes, I was not sure how much to edit all the old commit messages but in the future I will just leave one.

OK, this doesn't address my comments at #1634 (comment), but it does work. So let's merge it for now and I'll file a follow up issue.

one item I forgot to mention in email (re branch issues/1571/4 alt solution): for me the ideal solution would have been to handle/listen for the 2nd image drag/drop event and get the new file info from event, then update the file input field with the new file data. however, I followed the current docs (html drag and drop api) and was unable to get the file info from the event (maybe too experimental?). I can write up more details in the wiki if useful (mdn link, etc).

miketaylr · 2017-07-19T20:58:49Z

however, I followed the current docs (html drag and drop api) and was unable to get the file info from the event (maybe too experimental?).

That would be surprising, DnD is pretty old stuff at this point.

I can write up more details in the wiki if useful (mdn link, etc).

Sure, that sounds good.

cch5ng mentioned this pull request Jul 4, 2017

image dnd change [not ready to merge] #1616

Closed

miketaylr self-requested a review July 6, 2017 14:39

miketaylr added status: blocked status: do not merge labels Jul 6, 2017

miketaylr reviewed Jul 14, 2017

View reviewed changes

webcompat/static/js/lib/bugform.js Outdated

@@ -155,6 +156,18 @@ function BugForm() {

img.src = dataURI;

};

// Event handler for image file drop; purpose to detect multiple drops

this.handleImageDrop = function() {

var imageFilled = false;

This comment was marked as abuse.

Sign in to view

miketaylr reviewed Jul 14, 2017

View reviewed changes

webcompat/static/js/lib/bugform.js Outdated

@@ -522,8 +542,13 @@ function BugForm() {

img_url,

")"

].join("");

var delimeter = "[![Screenshot Description](";

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

miketaylr requested changes Jul 14, 2017

View reviewed changes

miketaylr removed status: blocked status: do not merge labels Jul 14, 2017

cch5ng force-pushed the issues/1571/1d branch from ef09716 to 9de3e8c Compare July 15, 2017 02:21

miketaylr requested changes Jul 18, 2017

View reviewed changes

cch5ng force-pushed the issues/1571/1d branch from 9de3e8c to 9a684d9 Compare July 19, 2017 04:34

miketaylr approved these changes Jul 19, 2017

View reviewed changes

miketaylr merged commit 1b8036c into master Jul 19, 2017

miketaylr deleted the issues/1571/1d branch July 19, 2017 17:39

miketaylr added the status: add to changelog label Jul 19, 2017

miketaylr removed the status: add to changelog label Jul 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1571 - handle multiple image drag and drop events #1634

Fixes #1571 - handle multiple image drag and drop events #1634

cch5ng commented Jul 4, 2017 •

edited

Loading

cch5ng commented Jul 4, 2017 •

edited

Loading

miketaylr commented Jul 6, 2017

miketaylr commented Jul 6, 2017

cch5ng commented Jul 6, 2017

miketaylr commented Jul 14, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 14, 2017

cch5ng commented Jul 14, 2017

cch5ng commented Jul 14, 2017

cch5ng commented Jul 15, 2017

miketaylr left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 18, 2017

cch5ng commented Jul 18, 2017 via email

cch5ng commented Jul 19, 2017

miketaylr commented Jul 19, 2017

cch5ng commented Jul 19, 2017

miketaylr commented Jul 19, 2017

Fixes #1571 - handle multiple image drag and drop events #1634

Fixes #1571 - handle multiple image drag and drop events #1634

Conversation

cch5ng commented Jul 4, 2017 • edited Loading

cch5ng commented Jul 4, 2017 • edited Loading

miketaylr commented Jul 6, 2017

miketaylr commented Jul 6, 2017

cch5ng commented Jul 6, 2017

miketaylr commented Jul 14, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 14, 2017

cch5ng commented Jul 14, 2017

cch5ng commented Jul 14, 2017

cch5ng commented Jul 15, 2017

miketaylr left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

miketaylr commented Jul 18, 2017

cch5ng commented Jul 18, 2017 via email

cch5ng commented Jul 19, 2017

miketaylr commented Jul 19, 2017

cch5ng commented Jul 19, 2017

miketaylr commented Jul 19, 2017

cch5ng commented Jul 4, 2017 •

edited

Loading

cch5ng commented Jul 4, 2017 •

edited

Loading