Update FileDropzone API #678

gilest · 2022-01-28T23:14:56Z

Adds new arguments queue and filter to FileDropzone.

Includes basic test coverage of these new APIs while deprecated API tests still pass.

Adds typing to FileDropzone component and DataTransferWrapper internal utility. I could have taken this further but I think it's good to do this process incrementally. Honestly there's a lot to untangle in some of these internal utilities (regarding typing support).

Registers/unregisters the FileDropzone as an event listener on the queue in the method from #654.

A note about naming

Following the work of @gossi in #620 had planned to name the callback args dragEnter, dragLeave and drop but there's an ember-template-lint rule specifically discouraging this.

I don't like the names I have chosen – will welcome other suggestions of how we name the callback arguments but I would very much like it to be consistent across the whole addon.

I've decided to revert to using existing event names with the on prefix.

gilest · 2022-01-28T23:28:15Z

addon/components/file-dropzone.ts

+    // @TODO - add tests for these or remove them
+    // // Testing support for dragging and dropping images
+    // // from other browser windows
+    // let url;
+
+    // // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // // @ts-ignore
+    // const html = this.dataTransferWrapper.getData('text/html');
+    // if (html) {
+    //   const parsedHtml = parseHTML(html);
+    //   // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //   // @ts-ignore
+    //   const img = parsedHtml.getElementsByTagName('img')[0];
+    //   if (img) {
+    //     url = img.src;
+    //   }
+    // }
+
+    // if (url == null) {
+    //   // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //   // @ts-ignore
+    //   url = this.dataTransferWrapper.getData('text/uri-list');
+    // }
+
+    // if (url) {
+    //   const image = new Image();
+    //   const [filename] = url.split('/').slice(-1);
+    //   image.crossOrigin = 'anonymous';
+    //   image.onload = () => {
+    //     const canvas = document.createElement('canvas');
+    //     canvas.width = image.width;
+    //     canvas.height = image.height;
+
+    //     const ctx = canvas.getContext('2d');
+    //     ctx?.drawImage(image, 0, 0);
+
+    //     if (canvas.toBlob) {
+    //       canvas.toBlob((blob) => {
+    //         // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //         // @ts-ignore
+    //         const [file] = this.addFiles([blob], FileSource.web);
+    //         // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //         // @ts-ignore
+    //         file.name = filename;
+    //       });
+    //     } else {
+    //       const binStr = atob(canvas.toDataURL().split(',')[1]);
+    //       const len = binStr.length;
+    //       const arr = new Uint8Array(len);
+
+    //       for (let i = 0; i < len; i++) {
+    //         arr[i] = binStr.charCodeAt(i);
+    //       }
+    //       const blob = new Blob([arr], { type: 'image/png' });
+    //       // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //       // @ts-ignore
+    //       blob.name = filename;
+    //       // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //       // @ts-ignore
+    //       const [file] = this.addFiles([blob], FileSource.web);
+    //       // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    //       // @ts-ignore
+    //       file.name = filename;
+    //     }
+    //   };
+    //   /* eslint-disable no-console */
+    //   image.onerror = function (e) {
+    //     console.log(e);
+    //   };
+    //   /* eslint-enable no-console */
+    //   image.src = url;
+    // }


This code concerns me as I have found there is no test coverage of it and I'm not sure how to test it manually.

I'm worried we may have broken this functionality without our knowledge in recent updates. However I did try to drag an image from another browser window in v4.0.3 and it didn't work so 🤷🏻‍♂️ .

Options:

Explicitly remove this as it was undocumented (although I think allowUploadsFromWebsites is related) and untested. Can always be added back as an non-breaking enhancement

Try and figure out how it works and support it now

addon/queue.ts

tests/helpers/file-queue-helper-test.js

addon/components/file-dropzone.ts

jelhan · 2022-02-01T06:49:08Z

I will try to do a full review later today. But thought that leaving some thoughts regarding naming early may make sense.

A note about naming

Following the work of @gossi in #620 had planned to name the callback args dragEnter, dragLeave and drop but there's an ember-template-lint rule specifically discouraging this.

Did you considered to keep the on* naming convention? For example onDragEnter and onDrop.

Prefixing event handlers with on helps with readability in my opinion. It communicates to the reader that this is an event handler, which executes a passed in function when a specific situation happens. It is also seems to be kind of a standard among favorite Ember addons.

It also helps to discover the available event handlers - especially when looking at autocompletion in templates. I hope that we will get to that in the foreseeable future. First-class component templates and Glimmer component signature type RFCs are very promising in that regard.

gossi

I love to see all this classic code go and see it replaced with new code.

I didn't look into the details of drag'n'drop code (as I don't know the details) but more on a "highlevel" view. Left some comments about the naming.

Apart from that, I think, it is looking really good.

addon/components/file-dropzone.ts

tests/integration/components/file-dropzone-test.js

gilest · 2022-02-18T23:03:22Z

@gossi @jelhan Thanks for your comments. I've opted to use the original callback names and the on prefix.

I think it's important that the callback names are consistent so I've created #701 to track.

Believe this is ready to merge and will do so in a day or two if I don't get further reviews.

refactor(data transfer): convert to typescript, add namespace

…dle (deprecated) multiple argument internally test(file queue): reduce mirage handler timing

…ch (deprecated) onFileAdd

Comment out experimental cross-window support. It's not covered by any tests. Not sure it works or is in use. Use FileSource enums rather than literals. Ensure that dropping multiple files is allowed by default.

docs(file upload): improve deprecated method recommendation

…opzoneWrapper - hopefully makes it more clear that this is an internal wrapper Simplify logic and clarify the function of FileDropzoneWrapper by creating a filesOrItems property. Improve typing of file property callback arguments.

feat(both components): remove unpublished filtering docs(validation): update validation doc to describe the `@filter` arg

They reappeared in the rebase.

gilest commented Jan 28, 2022

View reviewed changes

addon/queue.ts Outdated Show resolved Hide resolved

gilest commented Jan 28, 2022

View reviewed changes

tests/helpers/file-queue-helper-test.js Outdated Show resolved Hide resolved

gilest commented Jan 28, 2022

View reviewed changes

addon/components/file-dropzone.ts Outdated Show resolved Hide resolved

gilest requested review from gossi and jelhan January 28, 2022 23:32

gilest mentioned this pull request Jan 28, 2022

Roadmap to v5 #331

Closed

22 tasks

gossi reviewed Feb 1, 2022

View reviewed changes

gilest added the enhancement label Feb 18, 2022

gilest force-pushed the update-dropzone-api branch from 7ec3b29 to 0a5717c Compare February 18, 2022 21:50

gilest marked this pull request as ready for review February 18, 2022 21:50

gilest requested a review from gossi February 18, 2022 23:03

gilest added 12 commits February 22, 2022 09:51

refactor(file dropzone component): convert to typescript

0c6e782

refactor(data transfer): convert to typescript, add namespace

refactor(file dropzone component): drop update-queue modifier and han…

406dc8b

…dle (deprecated) multiple argument internally test(file queue): reduce mirage handler timing

refactor(file dropzone component): bind to queue listener and re-atta…

7708be9

…ch (deprecated) onFileAdd

refactor(file dropzone): Drop _addFiles from the Queue and reimplement.

0fb6049

Comment out experimental cross-window support. It's not covered by any tests. Not sure it works or is in use. Use FileSource enums rather than literals. Ensure that dropping multiple files is allowed by default.

test(file dropzone): add additional coverage for @mulitple

20f1ad3

refactor(file dropzone): implement new events API incl tests

8bc3500

docs(file dropzone): update usage example

9782e4c

docs(file upload): improve deprecated method recommendation

chore(dummy app): use new helper API

4720360

chore: update TODO comments where applicable

a89d410

fix(upload file): mistaken change to blob source/name

b36c04d

feat(file dropzone): restore on prefix to callback arguments

5d81caa

feat(both components): remove unpublished filtering docs(validation): update validation doc to describe the `@filter` arg

gilest force-pushed the update-dropzone-api branch from 0a5717c to 5d81caa Compare February 21, 2022 20:53

fix: remove additional data-transfer files

7e84342

They reappeared in the rebase.

gilest merged commit 840f8d9 into master Feb 21, 2022

gilest deleted the update-dropzone-api branch February 21, 2022 21:36

This was referenced Jan 19, 2024

Update documentation #1058

Closed

Document historic v5 removal – dragging images from other browser windows and @allowUploadsFromWebsites #1059

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update FileDropzone API #678

Update FileDropzone API #678

gilest commented Jan 28, 2022 •

edited

Loading

gilest Jan 28, 2022 •

edited

Loading

jelhan commented Feb 1, 2022

gossi left a comment

gilest commented Feb 18, 2022

Update FileDropzone API #678

Update FileDropzone API #678

Conversation

gilest commented Jan 28, 2022 • edited Loading

A note about naming

gilest Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

jelhan commented Feb 1, 2022

gossi left a comment

Choose a reason for hiding this comment

gilest commented Feb 18, 2022

gilest commented Jan 28, 2022 •

edited

Loading

gilest Jan 28, 2022 •

edited

Loading