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

File Upload component exposes dropzoneProps from react-dropzone, should instead abstract away those features #3966

Open
mturley opened this issue Mar 24, 2020 · 1 comment
Labels
PF4

Comments

@mturley
Copy link
Collaborator

@mturley mturley commented Mar 24, 2020

Describe the issue. What is the expected and unexpected behavior?

The new beta FileUpload component uses the react-dropzone library internally to handle the file browse and drag/drop behavior. The basic use case (single text file, no restrictions) is handled directly by our FileUpload props (e.g. value, filename, onChange, onReadStarted/Finished, etc) which determine what simple props are passed into the Dropzone.

All other features of Dropzone are made available by exposing an object prop called dropzoneProps which allows the consumer to pass whatever props they need to that are supported by Dropzone. The "Text file with restrictions" example in our docs demonstrates this, by passing the accept, maxSize and onDropRejected props directly in dropzoneProps, since we don't expose our own props for these things.

I was discussing this with @redallen and he made the point that exposing their props interface directly may not be a good idea, since we will be stuck with that interface and dependent on their documentation for any features that require it. If we ever wanted to move away from react-dropzone and use something else internally, or remain on an older version if we disagree with changes they make in the future, we'd be stuck with consumers using their old props and we'd have to make a large breaking change or have people confused why they can't use newer props from new react-dropzone docs.

His proposed solution is to remove the dropzoneProps escape hatch, and expose our own props matching a subset of the props they provide. We could copy and paste their prop descriptions from here: https://github.com/react-dropzone/react-dropzone/blob/master/src/index.js#L66

This way, all of the available props and documentation would be in-house, we would no longer need to depend on react-dropzone's docs, and if we needed to depart from react-dropzone's future changes we would be able to without necessarily breaking consumers.

The main drawback here would be that if there is some use case the dropzone props support that we choose not to expose, consumers would not be able to use that unless we update our props. Also, we would complicate our props table in the docs with a bunch of stuff many people will never use.

I'm leaning towards agreeing with @redallen here, but feedback is welcome.

Please provide the steps to reproduce. Feel free to link CodeSandbox or another tool.

N/A

Is this a bug or enhancement? If this issue is a bug, is this issue blocking you or is there a work-around?

Bug, sort of, more like potential technical debt.

What is your product and what release version are you targeting?

N/A

@mturley mturley added the PF4 label Mar 24, 2020
@mturley mturley changed the title File Upload component exposes reactDropzone props, should instead abstract away those features File Upload component exposes dropzoneProps from react-dropzone, should instead abstract away those features Mar 24, 2020
@mturley

This comment has been minimized.

Copy link
Collaborator Author

@mturley mturley commented Mar 24, 2020

This is sort of related to #3875, since react-dropzone v10 and up requires React 16.8+, so if we intend to continue upgrading react-dropzone we'll need to upgrade React.

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

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.