Feature/custom get data transfer items #616

MarinaZadoyanchuk · 2018-06-01T15:59:46Z

What kind of change does this PR introduce?

Did you add tests for your changes?

Yes, my code is well tested
Not relevant

If relevant, did you update the documentation?

Yes, I've updated the documentation
Not relevant

Summary

Here is a very basic idea of how plugin interface could be implemented (as was suggested by @okonet in #609), basically we are giving the user an ability to specify it's own function getDataTransferItems instead of default one. Would be great to hear some thoughts and suggestions on this.

Does this PR introduce a breaking change?
No

Closes #609

okonet · 2018-06-02T10:16:08Z

Great work on this! I really like how it's shaping.

Please reformat the code so CI is green. It should have happen during the pre-commit hook. Did you skip it?
The documentation needs to be updated to explain the change and we'll need example with reading directories.

okonet · 2018-06-02T10:17:39Z

Inviting @quarklemotion to review and comment. Also, it would be great to sync the release of the directories reader to fit the API.

okonet · 2018-06-02T10:21:43Z

src/index.js

+      fileList.forEach(file => {
+        if (!disablePreview) {
+          try {
+            file.preview = window.URL.createObjectURL(file) // eslint-disable-line no-param-reassign


I'd propose to create an additional "plugin" called getDataTransferItemsWithPreview and move this code there. Ideally the core of this library should be not concerned about anything but drag'n'drop.

I'd even go with a breaking change there since I believe most users don't need preview generation by default and it's a runtime cost and a potential memory leak.

It’s not clear to me about how getDataTransferItemsWithPreview should be separated into an additional plugin. In the previous implementation, we use to have disablePreview prop which would enable/disable preview generation, so if we will take this code away, how would the user be able to generate previews with a plugin? I mean, importing plugin and passing it as getDataTransferItems will replace original function with all the core functionality, in this case, we should also export default getDataTransferItems plugin so the user can make a composition of those, or maybe add an additional prop that will take function to invoke with file list after getDataTransferItems ?

Sorry if I am missing something!

@Nodman I think exporting the default implementation + additional plugins is a way to go. Composition should be the right way of doing this. We should also add examples of how to achieve this.

okonet · 2018-06-02T10:23:18Z

src/index.js

@@ -585,5 +606,6 @@ Dropzone.defaultProps = {
  disableClick: false,
  multiple: true,
  maxSize: Infinity,
-  minSize: 0
+  minSize: 0,
+  getDataTransferItems: defaultGetDataTransferItem


Let's add a signature of the function here as a JSDoc to make it easier for plugins authors.

I have described it in the propTypes section, should I describe it here too?

Oh sorry I thought that is the propTypes. No, the way you did it is perfect! Thanks!

quarklemotion · 2018-06-04T04:16:58Z

@okonet this implementation makes sense to me. I like the use of Promise.resolve(this.props.getDataTransferItems(evt)) which allows for both synchronous and async implementations of props.getDataTransferItems. With the implementation in this pull request, I don't think there are any changes required to html5-file-selector to support this plugin interface - the getDroppedOrSelectedFiles(event) exported function can be used as the getDataTransferItems prop. I'm happy to add a new exported function getDroppedOrSelectedFilesWithPreview(event) to the file selector module if everyone thinks that is the best place to locate this wrapper function to provide preview support?

okonet · 2018-06-04T06:08:03Z

I think the preview plugin should live in this repo. I’d even say your plugin should probably too. I’m not sure how much overhead setting up multiple packages would be, though. Yarn workspaces should solve it, right?

quarklemotion · 2018-06-05T15:59:00Z

@okonet Ah, I see - I think that could make sense to convert react-dropzone to multi-package. Were you thinking that the folder drop plugin package would be a small wrapper of html5-file-selector, or html5-file-selector would move into the react-dropzone repo? One thing to consider is that the file selector module was built to support both dropzones and file input fields, whereas react-dropzone only cares about the dropzone support. There are other projects using the file selector module, and I want to make sure they can continue to use it. As for multi-package setup, I haven't used yarn workspaces before, only lerna, but happy to help with this effort!

okonet · 2018-06-05T19:32:31Z

Totally makes sense to me. Let’s keep thing simple for now and add your module to documentation.

codecov · 2018-06-05T21:45:35Z

Codecov Report

Merging #616 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   99.01%   99.02%   +<.01%     
==========================================
  Files           3        3              
  Lines         204      205       +1     
  Branches       59       59              
==========================================
+ Hits          202      203       +1     
  Misses          2        2

Impacted Files	Coverage Δ
src/utils/index.js	`96.42% <ø> (ø)`	⬆️
src/index.js	`99.42% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28526fe...de79247. Read the comment docs.

okonet · 2018-06-15T07:21:48Z

I think the most important part we're missing is the documentation. Can you please add it and we could merge it as is. After that we could extract bits for preview.

MarinaZadoyanchuk · 2018-06-19T15:05:55Z

Ok, I'm going to write a usage of getDataTransferItems soon.

okonet · 2018-06-19T15:21:04Z

@MarinaZadoyanchuk thanks!

dejongch · 2018-07-16T19:49:45Z

Any update on when this might be merged in?

arunchouhan163 · 2018-08-06T09:50:59Z

Please merge this PR. ASAP.

Thanks.

okonet · 2018-08-06T13:26:51Z

Since this one is stalling, I decided to proceed and merge it. Anyone wants to work on the documentation? @quarklemotion do you want to add an example?

ralexrdz · 2019-02-13T03:24:32Z

I'm not succeding implementing getDataTransferItems. Do you have a example I can follow?

rolandjitsu · 2019-02-13T09:56:39Z

@ralexrdz here you go: using-third-party-plugins.

ralexrdz · 2019-02-13T21:13:31Z

@rolandjitsu thanks a lot. It was the Promise / async I was missing ;)

MarinaZadoyanchuk added 2 commits May 31, 2018 15:04

feat: support custom getDataTransferItems prop

64f1ba1

fix: reset docs editing

9d32dec

Nodman added help wanted question in progress labels Jun 1, 2018

MarinaZadoyanchuk requested a review from okonet June 1, 2018 16:01

Merge branch 'master' into feature/custom-get-data-transfer-items

cb59886

Nodman removed the question label Jun 1, 2018

fix: eslint error

22b3aff

okonet requested changes Jun 2, 2018

View reviewed changes

okonet mentioned this pull request Jun 5, 2018

Feature/folder support #594

Closed

9 tasks

MarinaZadoyanchuk added 2 commits June 6, 2018 00:17

chore: extended description

7065336

fix: downgrade eslint for correct pre-commit hook

0702dc4

This was referenced Jun 15, 2018

Handle folder drops recursively #11

Closed

Add support for selecting/dropping a directory #134

Closed

Allow selection (input or drop) of directories #174

Closed

Merge branch 'master' into feature/custom-get-data-transfer-items

83b52e1

Merge branch 'master' into feature/custom-get-data-transfer-items

3207ad7

okonet approved these changes Aug 6, 2018

View reviewed changes

Merge branch 'master' into feature/custom-get-data-transfer-items

de79247

okonet merged commit 71a54ad into react-dropzone:master Aug 6, 2018

asfordmatt mentioned this pull request Aug 15, 2018

v4.3.0 has introduced a breaking change to our unit tests #638

Closed

2 tasks

okonet mentioned this pull request Aug 24, 2018

chore upgrade to react 16 / enzyme 3 #645

Merged

9 tasks

This was referenced Sep 7, 2018

Do not call any of the onDrag*/onDrop handlers if the user is not dragging files #619

Merged

Add an example for folders drag'n'drop #661

Closed

sumthief mentioned this pull request Oct 8, 2018

event.stopPropagation in custom event handlers doesn't work #686

Closed

2 tasks

BuloZB mentioned this pull request May 20, 2024

[Snyk] Upgrade react-dropzone from 4.2.5 to 4.3.0 BuloZB/speech-to-text-nodejs#4

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/custom get data transfer items #616

Feature/custom get data transfer items #616

MarinaZadoyanchuk commented Jun 1, 2018 •

edited by okonet

okonet commented Jun 2, 2018 •

edited by MarinaZadoyanchuk

okonet commented Jun 2, 2018

okonet Jun 2, 2018

Nodman Jun 6, 2018 •

edited

okonet Jun 15, 2018

okonet Jun 2, 2018

MarinaZadoyanchuk Jun 5, 2018

okonet Jun 15, 2018

quarklemotion commented Jun 4, 2018 •

edited

okonet commented Jun 4, 2018

quarklemotion commented Jun 5, 2018

okonet commented Jun 5, 2018

codecov bot commented Jun 5, 2018 •

edited

okonet commented Jun 15, 2018

MarinaZadoyanchuk commented Jun 19, 2018

okonet commented Jun 19, 2018

dejongch commented Jul 16, 2018

arunchouhan163 commented Aug 6, 2018

okonet commented Aug 6, 2018

ralexrdz commented Feb 13, 2019

rolandjitsu commented Feb 13, 2019

ralexrdz commented Feb 13, 2019

Feature/custom get data transfer items #616

Feature/custom get data transfer items #616

Conversation

MarinaZadoyanchuk commented Jun 1, 2018 • edited by okonet

okonet commented Jun 2, 2018 • edited by MarinaZadoyanchuk

okonet commented Jun 2, 2018

okonet Jun 2, 2018

Choose a reason for hiding this comment

Nodman Jun 6, 2018 • edited

Choose a reason for hiding this comment

okonet Jun 15, 2018

Choose a reason for hiding this comment

okonet Jun 2, 2018

Choose a reason for hiding this comment

MarinaZadoyanchuk Jun 5, 2018

Choose a reason for hiding this comment

okonet Jun 15, 2018

Choose a reason for hiding this comment

quarklemotion commented Jun 4, 2018 • edited

okonet commented Jun 4, 2018

quarklemotion commented Jun 5, 2018

okonet commented Jun 5, 2018

codecov bot commented Jun 5, 2018 • edited

Codecov Report

okonet commented Jun 15, 2018

MarinaZadoyanchuk commented Jun 19, 2018

okonet commented Jun 19, 2018

dejongch commented Jul 16, 2018

arunchouhan163 commented Aug 6, 2018

okonet commented Aug 6, 2018

ralexrdz commented Feb 13, 2019

rolandjitsu commented Feb 13, 2019

ralexrdz commented Feb 13, 2019

MarinaZadoyanchuk commented Jun 1, 2018 •

edited by okonet

okonet commented Jun 2, 2018 •

edited by MarinaZadoyanchuk

Nodman Jun 6, 2018 •

edited

quarklemotion commented Jun 4, 2018 •

edited

codecov bot commented Jun 5, 2018 •

edited