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

High-level React Components #170

Merged
merged 40 commits into from
Sep 22, 2017

Conversation

goto-bus-stop
Copy link
Contributor

@goto-bus-stop goto-bus-stop commented Mar 17, 2017

This PR introduces three high-level React components that render a Dashboard or a DragDrop area for a given Uppy instance.

There's a plain <Dashboard /> component, that renders the Dashboard using the inline option. This is nice & flexible and works well to actually render the dashboard inline, but also to render it in a modal from some other library if one is in use in someone's project already.

There's a <DashboardModal /> component, that renders a Dashboard modal like react-less Uppy does. The difference is that it doesn't have a trigger option, and instead has an open prop. This way it can be opened or closed depending on state held in React or Redux and controlled manually by eg. a button and setState. When the user clicks outside the modal, the onRequestClose event is fired. The developer can then decide if and how the modal should be closed.

There's a <DrapDrop /> component, that renders the DrapDrop plugin. There's not much more to this ATM. A bunch of event callbacks for hover and drop may be useful here. These events would be actually related to the DOM that's rendered, and not as much to the Uppy Core instance, like with the onProgress/onComplete events (see below).


Todos:

  • The GoogleDrive/Dropbox acquirer plugins have a target: option which seems to have to be(?) a Dashboard plugin instance. This is difficult with how these React components work, since these add the Dashboard plugin inside the component. One way to work around this would be to have the Dashboard find the relevant acquirer plugins by itself (probably by iterating over Uppys internal plugin list).
  • Figure out the event callbacks game. Having onProgress/onComplete callbacks on the components might be useful, but these are also available as events on the Uppy instance, and having multiple ways to add them might be confusing. I'll play around with this a bit more in the Example to see how it feels to use the event emitter vs handlers on the React components. Just use uppy.on() for events
  • Make these components deal well with being mounted and unmounted. Currently the Uppy instance with any plugins added by components may outlive the component, and I'm not sure what happens then. We may need a way for Uppy to un-use a plugin…or just View plugins, really. Perhaps with an uninstall method.
  • Add the Dashboard maxWidth/maxHeight/semiTransparent/defaultTabIcon/showProgressDetails options as props.
  • Maybe just map all props to options by default, and only manually add some specific ones when needed?
  • Add locale props to all components.
  • Add <ProgressBar />
  • Rebase
  • Docs

And a separate thought re: the DragDrop component: A good story with eg. React-DnD seems useful. If there's an Uppy method to add a browser File object (like you get from input.files[0]) that is likely enough, though, and wouldn't need anything React-specific.

@arturi
Copy link
Contributor

arturi commented Mar 17, 2017

Thank you! I will review soon.

@arturi arturi self-requested a review March 17, 2017 22:48
@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Mar 20, 2017

I added a bit of an experiment for mounting/unmounting the <Dashboard /> component. It's kinda awkward internally right now, because it's removing plugins manually, but it works really well with React. It'd be nicer with a Core#removePlugin method, but I'm not sure if a method like that should be able to remove any plugin (say, Multipart or Tus10), or somehow just plugins that have views.

@oyeanuj
Copy link

oyeanuj commented Jun 7, 2017

@goto-bus-stop Wondering if these components are ready to merge? I am especially curious about playing with <DragDrop /> since it seems like a react-dropzone replacement.

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Aug 28, 2017

Rebased. I think we'll try to get these in during September(?)

@​oyeanuj, <DragDrop /> is a thin React wrapper around the DragDrop plugin, so it basically does do what you would otherwise use dropzone for, except using the entire page as a droppable area (we should add something to do that)

* will.
*/
class ReactDashboardPlugin extends DashboardPlugin {
constructor (core, opts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m a little worried about doing this. Could we maybe add onRequestClose/hideModal callback into the Dashboard plugin itself? Otherwise its third-level of class nesting, which is not that bad, but starting to feel like a dangerous territory :)

@goto-bus-stop
Copy link
Contributor Author

whoops, pushed an onRequestClose option quickly in response to your comment, but it doesn't work yet--have to run now, will get back to that later today or tomorrow.

@arturi
Copy link
Contributor

arturi commented Sep 3, 2017

Sure! Thanks.

@goto-bus-stop
Copy link
Contributor Author

Trying to address the first TODO by making Dashboard auto-add provider plugins to itself if they did not do so explicitly, i.e.:

uppy.use(GoogleDrive, { target: Dashboard }) // does not add target because no Dashboard plugin is known
uppy.use(Dashboard) // finds GoogleDrive plugin with `.opts.target===Dashboard` and adds it
uppy.use(Dropbox, { target: Dashboard }) // adds target in `mount()` like it always has

This works with React components like:

const DashboardPlugin = require('uppy/lib/plugins/Dashboard')
const Dashboard = require('uppy/lib/uppy-react/Dashboard')

const uppy = Uppy()
  .use(GoogleDrive, { target: DashboardPlugin })
  .run()
const Demo = () => (
  <Dashboard uppy={uppy} />
)

It's confusing unfortunately, because you have to target: DashboardPlugin which is a different Dashboard from the Dashboard component.
Also, to do this I had to turn mount() into a noop if the target plugin was not added. If you've typoed the target: PluginName or you legitimately forgot to .use() the target, we should throw an error and not just ignore it, but this change prevents us from throwing an error there in the future.

I was also thinking of making Dashboard auto-add all .type=='acquirer' plugins, and removing the target: option from provider plugins, but in that case provider plugins that are added after the Dashboard was added won't be detected.

Maybe to address the last point, we can add a core.on('use', Plugin) event.

@@ -95,6 +103,10 @@ module.exports = class Plugin {
}
}

focus () {
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need focus for something after all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's not being used anywhere. will remove, thanks!

DashboardModal.propTypes = {
uppy: PropTypes.instanceOf(UppyCore).isRequired,
open: PropTypes.bool,
onRequestClose: PropTypes.func
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onRequestClose doesn't work yet


UppyWrapper.propTypes = {
uppy: PropTypes.instanceOf(UppyCore).isRequired,
plugin: PropTypes.string.isRequired
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into making a custom proptype validator here to assert that the plugin exists

@goto-bus-stop
Copy link
Contributor Author

Trying a different approach to React components suggested by @arturi.
Instead of the components adding and removing plugins when mounting and
unmounting, plugins are configured at the start, and the components act
as slots for the plugins to actually mount in. Because all plugins are
still being configured up front here, we don't have to change how the
provider target: options work, they can install themselves into eg.
the Dashboard plugin immediately.

The core of this approach is the UppyWrapper component, which takes an
Uppy instance and a plugin ID, and mounts the plugin inside itself. The
other components only provide a default plugin ID.

The one change required in Uppy itself for this to work is to the
mount() functions: now, mount() configures this.target on the
plugins. If plugins do not have a target: option, they do not mount on
install. Some DOM code also had to be moved into the mount() function
for the DragDrop component instead of staying in install(), but I
think it makes sense.

(Still have to work out how to make the DashboardModal component's
onRequestClose option work.)

See the react-example for how this looks when used

@oyeanuj
Copy link

oyeanuj commented Sep 11, 2017

@goto-bus-stop Seeing this example made me think if you folks considered something (and rejected?) like below as a maybe more React way of using Uppy:

<Uppy 
  files = {this.state.files} 
  onUploadSuccess = {this.handleUploadSuccess} 
  onUploadComplete = {this.handleUploadComplete}
>
  <Tus10 endpoint = 'https://master.tus.io/files' />
  <DashboardPlugin inline />
  <DragDropPlugin locale = {localeStringObject} />
  <ProgressBarPlugin />
 <GoogleDrive target = {DashboardPlugin} host = 'https://server.uppy.io'  />
</Uppy>

And I'm curious if you tried it, what did you like/dislike about the approach?

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Sep 13, 2017

Briefly considered it, but there are some issues with that approach IMO:

React is primarily a view framework so putting all of the upload logic in that place feels wrong to me. I don't like having non-UI things in a React render method. It's possible to do it like that, and probably fine in many cases, but I think in a redux-like architecture the uploading should be done in a middleware or an action somewhere. Or even just explicitly in a component lifecycle method, if not using Redux.

Also, different UI plugins can mount to different places with Uppy, and it's not clear how that would work in React. In React, usually when you write <Element/>, you expect the element to be rendered in the corresponding spot in the DOM. An Uppy instance could have both a modal dialog to drop files into, and a small progress bar elsewhere in the page to show progress when the dialog is closed. Those would be two different uppy plugins, and they'd be mounted in different places.
One option to address that would be to have the main <Uppy/> component add the instance to context, similar to <Provider/> in Redux, and have plugin components down the tree take it from context, but then we have to think about how multiple instances would work, and where to put eg. the Tus plugin (with the UI or with the <Uppy/> component?).


We discussed the new approach in slack/hangouts on Monday and are now thinking of going back to the previous approach, where we have components for UI plugins. But instead of setting target options on the providers, you'd pass a list of provider IDs that should be used to the component.

const DashboardPlugin = require('uppy/lib/plugins/Dashboard')
const Dashboard = require('uppy/lib/uppy-react/Dashboard')

const uppy = Uppy()
  .use(GoogleDrive) // no target:
  .run()
const Demo = () => (
  <Dashboard
    note="Select some stuff from Google Drive"
    uppy={uppy}
    plugins={['GoogleDrive']} />
)

The downside is that you have to duplicate the 'GoogleDrive' name, but it's also nicely explicit, I think.

@oyeanuj
Copy link

oyeanuj commented Sep 13, 2017

@goto-bus-stop appreciate the detailed response 😄! I think your points are fair. I have to say though that I consider React to be more than a view framework with the idea of self-encapsulated components, so in that vein, the above idea that I proposed could make sense.

Increasingly, a lot more components are being published that don't render anything by default - either act like an HoC or provide some functionality, like ReactRouterv4 or ReactDropzone, so Uppy could pick that lineage.


At the end of the day, for a more seamless and natural integration into a React-Redux environment, my wishlist would be that it either acts like a self-encapsulated black-box with hooks like in the idea above, or provides individual functions that a developer can use within their lifecycle functions, and/or action-creators, but not necessarily have any hidden black-boxiness to it (not to beat a dead horse, since I know we discussed that before with @hedgerh ).


Thanks for being open to the idea!

@arturi
Copy link
Contributor

arturi commented Sep 13, 2017

I think we should at least finish and publish the current proposal ☝️, and then maybe experiment with more-react-like individual components later.

We do have <ProgressBar /> already, could add a <StartUpload /> or something similar. This is what Fine Uploader is doing: https://github.com/FineUploader/react-fine-uploader. But its not really what Anuj is proposing.

@oyeanuj
Copy link

oyeanuj commented Sep 13, 2017

@arturi 👍

As far as FineUploader components are concerned, I think a critical difference in my proposal (which I am not super attached to btw) is that in my proposal, the main Uppy object is a component and behaves like one, even if it doesn't render anything.

If you add <Thumbnail />, <ProgressBar /> components, then it can render something. This setup makes it for a much cleaner implementation where a user doesn't have to muck around with Uppy objects in componentDidMount etc.

And then this setup also allows you to have plugins as HoCs in the future... 🚀

In the case of FineUploader, it uses React components only for display or view elements.

@goto-bus-stop
Copy link
Contributor Author

TODO:

  • rebase
  • docs

If the dashboard is used in inline mode, the z-indexes are not
necessary, and may even interfere. This is visible in the React
demo, where the inline dashboard would appear on top of the overlay
of the modal dashboard, instead of behind it.
This implements unmounting for the inline Dashboard component.
To do that, the Dashboard plugin now has a method `uninstall`,
which removes all listeners and removes the Dashboard element
from the DOM. The `componentWillUnmount` handler removes the plugin
from Uppy's internal plugin list. If this approach is the way to
go, we'd add a method to core to remove plugins… I think in that
case, it'd be good to add some concept of View Plugins, so that
Acquirer plugins function like they do now but View plugins can be
mounted and unmounted.
This is a different approach to React components suggested by @arturi.
Instead of the components adding and removing plugins when mounting and
unmounting, plugins are configured at the start, and the components act
as slots for the plugins to actually mount in. Because all plugins are
still being configured up front here, we don't have to change how the
provider `target:` options work, they can install themselves into eg.
the Dashboard plugin immediately.

The core of this approach is the `UppyWrapper` component, which takes an
Uppy instance and a plugin ID, and mounts the plugin inside itself. The
other components only provide a default plugin ID.

The one change required in Uppy itself for this to work is to the
`mount()` functions: now, `mount()` configures `this.target` on the
plugins. If plugins do not have a `target:` option, they do not mount on
install. Some DOM code also had to be moved into the `mount()` function
for the DragDrop component instead of staying in `install()`, but I
think it makes sense.

(Still have to work out how to make the `DashboardModal` component's
`onRequestClose` option work.)
@goto-bus-stop goto-bus-stop changed the title [WIP] High-level React Components High-level React Components Sep 22, 2017
@goto-bus-stop goto-bus-stop merged commit 9af6b2f into transloadit:master Sep 22, 2017
Copy link
Member

@kvz kvz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hot 🔥

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.

4 participants