Drag & drop by dnarcese · Pull Request #71 · mozilla/send

dnarcese · 2017-06-08T20:14:17Z

Added drag & drop as well as a delete file confirmation.

Closes #61

ericawright

The delete popup could use some more styling work - you could add it to a separate PR if you wanted. "delete" and "nevermind" both look like the same button. Need:

a noticeable hover effect on each
click away from the popup and it closes

ericawright · 2017-06-08T20:21:43Z

frontend/src/upload.js

-      btn.addEventListener('click', e => {
+
+      // delete file
+      console.log($popupText.first());


nit: remove the console.logs

ericawright · 2017-06-08T20:33:13Z

frontend/src/upload.js

          localStorage.getItem(info.fileId)
        ).then(() => {
-          e.target.parentNode.parentNode.remove();
+          e.target.parentNode.parentNode.parentNode.parentNode.remove();


With jQuery you could try $(e.target).parents('tr')
with vanilla JS e.target.closest('tr')

ericawright · 2017-06-08T20:33:45Z

frontend/src/upload.js

+      // hide popup
+      $popupText.find('.nvm').click(e => {
+        $popupText.toggleClass('show');
+      });


these two have the same fn after the click event, perhaps we name the fn and reuse it?

ericawright · 2017-06-08T20:34:32Z

views/index.handlebars

          <th width=7%>Delete</th>
        </tr>
-        <div data-role="popup" id="popupArrow" data-arrow="true">
+        <!-- <div class="popup">


nit: remove commented code

ericawright · 2017-06-08T20:35:19Z

frontend/src/upload.js

+    } else {
+      file = event.target.files[0];
+    }
    let fileList = $('#uploaded-files');


nit $ in front to follow jquery convention

ericawright · 2017-06-08T20:37:55Z

public/main.css

+   z-index: 1;
+   bottom: 125%;
+   left: 50%;
+   margin-left: -80px;


do you need both left and margin-left here?

ericawright · 2017-06-08T20:39:28Z

public/main.css

+/* Toggle this class when clicking on the popup container (hide and show the popup) */
+.popup .show {
+   visibility: visible;
+   -webkit-animation: fadeIn 1s;


shouldn't need -webkit, it's old syntax
Can we change this to a transition, my understanding is that it is more efficient than a keyframes animation.

pdehaan · 2017-06-08T21:52:18Z

frontend/src/upload.js

+    event.preventDefault();
+    let file = '';
+    if (event.type == 'drop') {
+      file = event.dataTransfer.files[0];


Does this work if I drop multiple files on the drop target from Finder? Or will it only handle the first image?

Good question. If it doesn't we can follow up with a solution in another PR

Spoiler alert: It doesn't.

But it looks like we can do Array.from().forEach() (or just a regular for loop) on the FileList as a workaround. But that exposes another dull edge case where you're dragging 2-3 files onto the app and it returns you a single share URL that you can copy to your clipboard.

window.onUpload = event => { event.preventDefault(); let fileList = $('#uploaded-files'); let files = []; if (event.type === 'drop') { files = event.dataTransfer.files; } else { files = event.target.files; } Array.from(files).forEach(file => { ... });

dannycoates

I've got no additional changes

dannycoates · 2017-06-08T22:00:37Z

server/portal_server.js

    .then(err => {
      if (!err) {
        console.log('Deleted.');
+        res.sendStatus(200);


nice catch :)

dannycoates · 2017-06-08T22:02:06Z

frontend/src/upload.js

+    event.preventDefault();
+    let file = '';
+    if (event.type == 'drop') {
+      file = event.dataTransfer.files[0];


Good question. If it doesn't we can follow up with a solution in another PR

ericawright · 2017-06-09T14:08:12Z

frontend/src/upload.js

+        toggleShow();
+      });
+      // hide popup
+      $popupText.find('.nvm').click(e => {


btn.addEventListener('click', toggleShow); $popupText.find('.nvm').click(toggleShow);

ericawright · 2017-06-09T14:09:51Z

just the one more nit and then I think we're good to merge :)

dnarcese added 2 commits June 8, 2017 16:05

add drag and drop and delete confirmation

796577e

format

90809bf

dnarcese requested review from dannycoates and ericawright June 8, 2017 20:14

ericawright suggested changes Jun 8, 2017

View reviewed changes

return 200 from server on successful delete

42be528

pdehaan reviewed Jun 8, 2017

View reviewed changes

dannycoates reviewed Jun 8, 2017

View reviewed changes

pdehaan mentioned this pull request Jun 8, 2017

Download URLs don't wrap in Google Chrome #73

Closed

dannycoates mentioned this pull request Jun 8, 2017

Adding basic ESLint config #54

Merged

dnarcese and others added 2 commits June 9, 2017 09:45

making changes

dea589d

Merge branch 'master' into ui

b442b65

ericawright reviewed Jun 9, 2017

View reviewed changes

dnarcese added 3 commits June 9, 2017 10:47

reformat click listeners

3505574

update branch

9e77e90

change animation to transition

2323b17

dnarcese merged commit e077160 into master Jun 9, 2017

pdehaan mentioned this pull request Jun 9, 2017

Upload multiple files #75

Closed

Conversation

dnarcese commented Jun 8, 2017

Uh oh!

ericawright left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannycoates left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericawright commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants