Lightbox spinner: Show spinner in Lightbox when slow to load

[AdityaHirapara]

Previously, Lightbox remains black until an image is loaded from the server. Spinner will be shown to user in Lightbox, while an image is being loaded.

Fixes #2896.

[borisyankov]

Why did you choose 0 and 1 instead of boolean?

[AdityaHirapara]

Because I want to set flex value for PhotoView according to the current state(0 or 1), so that spinner can be displayed at center.

[borisyankov]

You don't need to use this form of setState if you are not going to use the previous value of the state.

[borisyankov]

There is a loadingIndicatorSource property of PhotoView (and the built-in Image component) that does implement this functionality.

I was considering a better option to a simple loading indicator to be to show the thumbnail stretched so we get a 'progressive loading' effect.

[jainkuniya]

Thanks @AdityaHirapara for the PR!
I would also suggest you to use loadingIndicatorSource. This till save manual styling (which is hard to maintain as there are high chances of breaking styles on different platforms with a small change) and showing loader by toggling var in state.

Thanks again!

[AdityaHirapara]

@borisyankov @jainkuniya, Actually, loadingIndicatorSource is only to show any default image as a placeholder, I can't show any component like loadingSpinner there using this property.
And also there are lots of issues regarding to this property of PhotoView like this: alwx/react-native-photo-view#35

[gnprice]

Thanks @AdityaHirapara for sending this!

The screencap looks good -- doing something with a thumbnail / a low-res version would be still better, but this is a solid improvement over what we have.

I agree with the code comments above. Would you send an updated version with those changes?

One more comment on the code: the flex: 0 as a way of making the PhotoView not show up in the layout feels a little nonobvious to me -- like I'd have to think a bit hard about the other things in the layout to be sure it'll exactly have that effect, and I'm not sure the answer is that it definitely always will. Can we find a more direct way of controlling that?

For example, how about display: none?

[AdityaHirapara]

Thanks @AdityaHirapara for sending this!

The screencap looks good -- doing something with a thumbnail / a low-res version would be still better, but this is a solid improvement over what we have.

I agree with the code comments above. Would you send an updated version with those changes?

One more comment on the code: the flex: 0 as a way of making the PhotoView not show up in the layout feels a little nonobvious to me -- like I'd have to think a bit hard about the other things in the layout to be sure it'll exactly have that effect, and I'm not sure the answer is that it definitely always will. Can we find a more direct way of controlling that?

For example, how about display: none?

@gnprice I have made those changes. Also, now controlling 'PhotoView' with display instead of flex as suggested. Please review the changes.

[gnprice]

Thanks @AdityaHirapara for the PR and the fast reply! Sorry this reply of mine is much less fast. :-)

There's just one thing I'm puzzled about when rereading this code:

 const styles = StyleSheet.create({
   img: {
-    height: 300,
     flex: 1,
   },

Do we really want to drop the height property here? What happens if the image is very tall, or very short?

If we do want this change, it seems like it can be understood separately from the rest of the changes, and so should be a separate commit. And in any case we'd definitely want to mention in the commit message that this change is happening and say why.

I'm guessing the reason it's there is that in a previous version of the PR where we used flex: 0 to hide the PhotoView, it was part of making that work. If so, that bears out the concern I had about that approach 🙂 :

the flex: 0 as a way of making the PhotoView not show up in the layout feels a little nonobvious to me -- like I'd have to think a bit hard about the other things in the layout to be sure it'll exactly have that effect, and I'm not sure the answer is that it definitely always will.

So I think probably if we just drop that line from the diff (i.e. keep that line unchanged in the code), this change will be good.

The work needed to merge this is just to rebase, drop that line, and then retest that the code works as intended.