Add onSelectPhoto prop to UnslashPicker

[calummonro]

Adds a new prop onSelectPhoto (type: function) to the <UnsplashPicker />.

This function is called when a photo is selected, and is called with the photo data from the Unsplash API.

[danielma]

Fixes #6

[danielma]

Thanks for the PR! This definitely solves the particular issue, but I do wonder if this might be possible using an adaptation of the InsertIntoApplicationUploader?

The uploader API as-is effectively calls this onSelectPhoto prop, and it would be possible to write an uploader that could trigger a state change on a parent component whenever it gets mounted.

Is that an adequate solution for your use case?

[calummonro]

No problem!

I'm not sure I understand 100% - are you talking about adding a new Uploader and/or adding the onSelectPhoto prop to the uploaders?

I feel like this should be independent of the uploaders. For my use case at least, I just need to know when a <Photo> was clicked. The type of uploader isn't important here - and whether the callback is called before or after the upload starts also feels unimportant.

I'm happy to go down the uploader API route if that is the preferred way (I may need a little guidance), but it feels like it might get a bit more complicated than it needs to be. The nice thing about doing it this way is it should work for all Uploaders.

Let me know which way you want to go with it.

[calummonro]

I also forgot to bump the package.json version - I'll do that now.

[danielma]

I'm not sure I understand 100% - are you talking about adding a new Uploader and/or adding the onSelectPhoto prop to the uploaders?

I think (if I understand your use case) that you could use the InsertIntoApplicationLoader

If you pass InsertIntoApplicationLoader as the Uploader prop, you can use the onFinishedUploading prop to get the behavior you want. I realize that may seem like an extra step, but that part of the API is designed to make sure the project interacts correctly with the rules for the Unsplash API. That "upload" step doesn't necessarily have to actually do anything with the photo, but it's important to "download" the photo from unsplash so the photographer gets credit and they can update their stats.

Looking at this example: https://github.com/danielma/unsplash-react/blob/9fbb0bb7e535a19ede15b7e99e9861f2d6a4831a/examples/index.js#L100

I think you could change the render function to only render the modal if this.state.imageUrl is null

[calummonro]

Thanks for the advice. Would this work using the BlobUploader? The issue with the onFinishedUploading prop is that it is not called synchronously (at least for the BlobUploader, which is our Uploader of choice).

I think I'm going to re-think this over the weekend anyway. My onSelectPhoto approach isn't great right now anyway, since if we choose to close the Modal (or unmount the <UnsplashPicker /> some other way), the internal async functions still fire and attempt to call setState on the unmounted component.

I might look into a prop for telling the <UnsplashPicker /> component to cancel these setState calls. I think for my use case I really want this component to accommodate the following lifecycle:

1. Component is mounted
2. Photo is clicked
3. Side-effects are kicked off
4. Component is safe to unmount

In short, in this code:

handleFinishedUploading = response => {
  this.setState({ loadingPhoto: null })
  this.props.onFinishedUploading(response)
}

I'd like to bypass the setState call if I know I'm going to unmount the component during the onSelectPhoto callback.

Let me know if this is something you want to avoid, or if it's worth pursuing.

[danielma]

Would this work using the BlobUploader?

Yep, it should. BlobUploader takes onBlobLoaded, which you could use to perform the changes you need

I think the best way to approach what you're looking for, given the async issues, is to do your application state change in onFinishedUploading, since that's the end of the callback change and at that point <UnsplashReact> should be safe to unmount

[calummonro]

I think the best way to approach what you're looking for, given the async issues, is to do your application state change in onFinishedUploading, since that's the end of the callback change and at that point should be safe to unmount

The issue with both of those props (onBlobUploaded and onFinishedUploading) is that they are called some time after the <Photo> was clicked - the purpose of this new prop is to provide a callback which is called immediately.

[danielma]

I think I understand what you're looking for, but the reason there's a delay is because the Unsplash API terms require users to explicitly download a photo, to make sure the stats are correct. So there's a separate request that has to be made before the component is "done."

We're kind of running into a weird limit of react design patterns, where the component functionality is tied to the UI. Of course, it doesn't have to be this way, but it's particular to these components.

[calummonro]

Yeah, I see your point. But I don’t think that necessarily prevents us from building in this functionality.

The issue (for my use case) is that the top-level component calls setState to null out the selected photo, when the photo has finished uploading. How about having a separate ‘fire-and-forget’ mode (set via a prop?) which skips this setState call. (I appreciate there may be more of these to skip).

I don’t believe it would be bad to have the component kick off an async function then immediately unmount.

In my case the ‘onFinishedUploading’ prop would still be used to dispatch a redux action (so I’m still looking to do the second request/image download).

Edit: typing on phone and hit close and comment halfway through

[calummonro]

Sorry, was playing around and forgot this PR would be updated. Any thoughts on my last comment?

[calummonro]

Hey danielma,

I've added a new commit which prevents the setState call if the onSelectPhoto prop is defined. It's not ideal, but I can try to make this more generic if it's something you want to continue with.

If this onSelectPhoto prop is used, then we can't guarantee that the 'host' app won't immediately unmount the <UnsplashPicker /> component. I appreciate this might not be ideal for general use. (In particular the current change won't work for users who would like to use the onSelectPhoto prop but who will not be immediately un-mounting the component).

How about this alternative - the ability to throw up a loading screen which says something along the lines of 'Crediting the artist...' while the second API request is being made. Then the loading screen disappears once all the uploading stuff has finished.

My main problem at the moment is that I can click a bunch of photo's in the time it takes for handleBlobUploaded to be called. The alternative would prevent this problem and I could unmount the <UnsplashPicker /> during the onBlobUploaded or onFinishedUploading callbacks. (To write this logic in my application I would still require an onSelectPhoto prop to trigger the loading screen. But we could just do this internally based on some new prop.)

The first method (having access to an onSelectPhoto prop is preferred, since then I can optimistically render the image while our application processes it.

[danielma]

I think I'm still just on the verge of really understanding the intricacies of your issue.

Would you be pulling to put together a demo that illustrates the the issue you're having and why it can't be solved with existing APIs? I'm not opposed to changing the API, but I'm still not quite sure why it needs to be changed

[calummonro]

No worries. I'll put a demo together over the next couple of days and get back to you.