Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent onDrop from being overridden #52

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alukach
Copy link

@alukach alukach commented Feb 5, 2018

Currently, defining onDrop in props overrides the Dropzone component's onDrop parameter due to the spread props.

I believe that this should fix #46.

@traveled1
Copy link

Thanks so much!! Will pull the latest!! :)

@ilyakatz
Copy link
Contributor

Just tried it out and works great, would be good to merge it in

@ilyakatz
Copy link
Contributor

ilyakatz commented Mar 15, 2018

Actually, if the onDrop is used for validation, what do you think of something like this

    if (!this.props.onDrop || this.props.onDrop(files, rejectedFiles)) {
      new S3Upload(options);
    }

And require onDrop to return true/falsy value?

@alukach
Copy link
Author

alukach commented Mar 15, 2018

@ilyakatz can you describe what the advantage would be for this second method?

@ilyakatz
Copy link
Contributor

According to this discussion, it looks like creators of the module want to use onDrop for validation. So if validation fails, we should tell react-dropzone-s3-uploder not to upload file into s3

@alukach
Copy link
Author

alukach commented Mar 16, 2018

I don't think any change in logic is necessary, this is a tiny bugfix. I believe that, with this fix, the module works as intended.

@ilyakatz
Copy link
Contributor

Well, current fix is definitely better than nothing, but it doesn't allow for validation of file prior to upload to S3, which is what it seems onDrop is supposed to do

@alukach
Copy link
Author

alukach commented Mar 16, 2018 via email

@alukach
Copy link
Author

alukach commented Dec 17, 2018

@founderlab any input on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing in onDrop prop causes handleDrop to be bypassed
3 participants