Show rejected styles when dragging multiple files and multiple==false #410

plee-nm · 2017-04-19T22:15:58Z

…dragged in multi mode false.

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

ATM setting multiple to false and dragging multiple files into dropzone will display "accepted" styles although only the first file will be accepted and the rest will be skipped.

Changes:

onDrop will be called with first file as acceptedFiles argument and the rest of files as rejected.
To align the implementation with accept prop it will also display rejected styles in this case.

Closes #409

Does this PR introduce a breaking change?
Not really

Other information
Closes #409

…ragged in multi mode false.

codecov · 2017-04-19T22:20:03Z

Codecov Report

Merging #410 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #410      +/-   ##
==========================================
+ Coverage   97.59%   97.63%   +0.04%     
==========================================
  Files           3        3              
  Lines         166      169       +3     
  Branches       41       41              
==========================================
+ Hits          162      165       +3     
  Misses          4        4

Impacted Files	Coverage Δ
src/index.js	`97.41% <100%> (+0.08%)`	⬆️
src/getDataTransferItems.js	`100% <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 3a7042f...b90171b. Read the comment docs.

okonet · 2017-04-20T08:42:30Z

src/index.js

@@ -79,11 +79,14 @@ class Dropzone extends React.Component {
      this.dragTargets.push(e.target);
    }

-    const allFilesAccepted = this.allFilesAccepted(getDataTransferItems(e, this.props.multiple));
+    // validate all files regardless if they are in multi file mode.
+    const files = getDataTransferItems(e, true);


Shouldn't it be aligned with https://github.com/okonet/react-dropzone/pull/410/files#diff-1fdf421c05c1140f6d71444ea2b27638R133?

true is default argument, so you can skip it

okonet · 2017-04-20T08:44:55Z

src/index.js

+    // validate all files regardless if they are in multi file mode.
+    const files = getDataTransferItems(e, true);
+    const allFilesAccepted = this.allFilesAccepted(files);
+    const isMultipleNotAllowed = this.props.multiple ? false : files.length > 1;


Let's remove negation from the variable name please

This can be rewritten as !this.props.multiple || files.length > 1

I removed the negation with the new changes.

okonet · 2017-04-20T08:46:01Z

src/index.js


    this.setState({
      isDragActive: allFilesAccepted,
-      isDragReject: !allFilesAccepted
+      isDragReject: !allFilesAccepted || isMultipleNotAllowed


Can allFilesAccepted be set accordingly before so this code remains intact?

Not sure what this means exactly. I just pushed a change so let me know if I need to update it.

okonet · 2017-04-20T08:46:31Z

This seems to be a breaking change, right?

plee-nm · 2017-04-20T14:06:27Z

@okonet Yeah it is a breaking change. What are your thoughts on adding the extra files on drag to the rejected files or all the files as rejected if it is multiple? Not sure how everyone feels about this but I don't want to have the user experience of a drop of N number of files and it only picking one and saying it is valid.

We can do this as breaking change or we can add another config like allowMultipleDrop and default it to true. Setting it to false would show the error class on drag and show any extra files as rejected files. Or we label it as a breaking change and bump the major version. Thoughts? I can submit a PR with these changes.

plee-nm · 2017-04-20T14:11:20Z

@okonet I would push for just making a breaking change, the code will be cleaner and I think the behavior is better for the end user. Also the breaking change would only affect users using multiple=false and it can be easily documented in a change log.

okonet · 2017-04-20T14:58:53Z

I think it makes sense to make a major version bump. The change you're proposing makes it more logical.

plee-nm · 2017-04-20T15:35:25Z

Alright great I'll submit a pr with these changes.

…ment per review comments.

plee-nm · 2017-04-20T16:24:46Z

@okonet I just pushed the changes needed to add extra files to rejected when multiple is set to false. Please let me know what the process is for a major version bump.

okonet

Great work! We're almost there.

okonet · 2017-04-24T21:17:14Z

src/index.js

@@ -159,6 +158,11 @@ class Dropzone extends React.Component {
      }
    });

+    if (!multiple) {
+      // if not in multi mode add any extra accepted files to rejected.  This will allow end users to easily ignore a multi file drop in "single" mode.
+      rejectedFiles.push(...acceptedFiles.splice(1));


Can this be rewritten with array destructuring?

Change acceptedFiles to a let so it can be redefined and then do something like

const [accepted, ...rejected] = acceptedFiles; rejectedFiles.push(...rejected); acceptedFiles = [accepted];

Or just leave it as it is?

Let's leave as is. Thanks for clarifying.

okonet · 2017-04-24T21:17:29Z

src/index.js

@@ -159,6 +158,11 @@ class Dropzone extends React.Component {
      }
    });

+    if (!multiple) {
+      // if not in multi mode add any extra accepted files to rejected.  This will allow end users to easily ignore a multi file drop in "single" mode.


Please split the comment into 2 lines

okonet · 2017-04-24T21:20:10Z

src/index.spec.js

@@ -455,44 +455,58 @@ describe('Dropzone', () => {
      expect(dropzone.state().isDragReject).toEqual(false);
    });

-    it('should expose acceptedFiles and rejectedFiles to children', () => {
+    it('multiple false valid files adds to rejected files on a multple drop', () => {


Can we still keep shoulds in tests?

okonet · 2017-04-24T21:21:08Z

Don't worry about the version bump. I'll take care of this during the merge!

okonet · 2017-04-25T18:00:31Z

Hey @plee-nm I've just added some more examples. Could you please check you branch out, run npm start and test all the examples if they still work? Since you can live-change the examples, try out different states.

plee-nm · 2017-04-25T18:15:20Z

@okonet Just ran through the all the examples, looks like everything is working.

okonet · 2017-04-25T19:19:45Z

I'm looking at how it works now and it doesn't seem to be any breaking changes. We'll still call onDrop with the single file. I think it can be released as patch. Am I missing something?

plee-nm · 2017-04-25T19:27:21Z

@okonet it is kind of a gray area because it is fixing undefined behavior. Making it a major would make sure no one that is relying on the undefined behavior is broken. Patch or Major I would have some kind of release note saying: when multiple=false on a multiple file drag it will now show the reject styles/class and add any extra files to the rejected files.

okonet · 2017-04-26T08:24:35Z

Thanks for clarification. I think note is enough since we aren't breaking the API.

Add fix for 409. Show the error class when multiple files are being d…

8e8be7c

…ragged in multi mode false.

okonet requested changes Apr 20, 2017

View reviewed changes

okonet changed the title ~~409 Show the error class when on multi file drag when multi=false~~ Show rejected styles when dragging multiple files and multiple==false Apr 20, 2017

plee-nm added 3 commits April 20, 2017 10:39

Remove the negation of the variable name and remove the ternary state…

602eb30

…ment per review comments.

remove multiple param to getDataTransferItems

f0d3593

Add to rejected files when multiple files are dropped in multiple=false

75e03cc

okonet requested changes Apr 24, 2017

View reviewed changes

break comment into multi line, word tests better with should

91b0373

okonet approved these changes Apr 25, 2017

View reviewed changes

Merge branch 'master' into master

b90171b

okonet merged commit 72a9d93 into react-dropzone:master Apr 26, 2017

okonet mentioned this pull request Jul 19, 2017

Reject all if multiple prop is false and multiple files are dropped #458

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show rejected styles when dragging multiple files and multiple==false #410

Show rejected styles when dragging multiple files and multiple==false #410

plee-nm commented Apr 19, 2017 •

edited by okonet

codecov bot commented Apr 19, 2017 •

edited

okonet Apr 20, 2017

okonet Apr 20, 2017

plee-nm Apr 20, 2017

okonet Apr 20, 2017

plee-nm Apr 20, 2017

okonet commented Apr 20, 2017

plee-nm commented Apr 20, 2017

plee-nm commented Apr 20, 2017

okonet commented Apr 20, 2017

plee-nm commented Apr 20, 2017

plee-nm commented Apr 20, 2017 •

edited

okonet left a comment

okonet Apr 24, 2017

plee-nm Apr 24, 2017

okonet Apr 25, 2017

okonet Apr 24, 2017

okonet Apr 24, 2017

okonet commented Apr 24, 2017

okonet commented Apr 25, 2017

plee-nm commented Apr 25, 2017

okonet commented Apr 25, 2017

plee-nm commented Apr 25, 2017

okonet commented Apr 26, 2017

Show rejected styles when dragging multiple files and multiple==false #410

Show rejected styles when dragging multiple files and multiple==false #410

Conversation

plee-nm commented Apr 19, 2017 • edited by okonet

codecov bot commented Apr 19, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet commented Apr 20, 2017

plee-nm commented Apr 20, 2017

plee-nm commented Apr 20, 2017

okonet commented Apr 20, 2017

plee-nm commented Apr 20, 2017

plee-nm commented Apr 20, 2017 • edited

okonet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okonet commented Apr 24, 2017

okonet commented Apr 25, 2017

plee-nm commented Apr 25, 2017

okonet commented Apr 25, 2017

plee-nm commented Apr 25, 2017

okonet commented Apr 26, 2017

plee-nm commented Apr 19, 2017 •

edited by okonet

codecov bot commented Apr 19, 2017 •

edited

plee-nm commented Apr 20, 2017 •

edited