Routing support?

[oyeanuj]

Hi @neptunian! Thank you for the component!

Do you have thoughts on the component allowing for router support - to have it update the url as one opens the lightbox and moves from one image to another?

I noticed you are also the maintainer of react-images now. Would that be a more appropriate library to build this feature on?

Thank you!

[neptunian]

@oyeanuj Yes, definitely. I've been wanting to implement this. I'll give it some thought of where best to implement. Thanks.

[GregoryPotdevin]

Routing is easy to add if components are stateless (like react-images is).

It would be nice to also have a stateless version of the the gallery that only displays the images and has an onImageClick prop. The you could use your own router, display the Gallery, update the route on onImageClick and on the Lightbox's gotoPrev/gotoNext.

It's nice to have a stateful version that just works, but when you want to add routing externalising state makes it easier.

[neptunian]

Thanks @GregoryPotdevin , I'll think that over and see what I come up with.

[dmr]

Did you come up with a good idea for this feature in the meantime?

[neptunian]

@dmr yes im just going to expose the lightbox methods as Gregory above stated so the user can handle routing on their own. I'll commit something soon.

[neptunian]

@dmr @GregoryPotdevin @smeijer I was thinking to pass in all the lightbox options as a Gallery property. So I'd be removing all the methods that Lightbox uses such as gotoNext, gotoPrev, closeLightbox etc and the user would have to manage the state and provide the methods in their own app. What do you think? Any input/guidance/sugestions on this would be greatly appreciated!

[smeijer]

A direct pass trough to the lightbox lib seems to be great. That way it's more easy to upgrade this project and it's dependencies as well. Whenever lightbox might add a feature; you can simply update the dependency and be done with it.

+1 for a simple pass trough.

[neptunian]

@smeijer @dmr @GregoryPotdevin Okay I've implemented changes and have things working locally. Here is a gist of the updated files if anyone has any comments before I commit: https://gist.github.com/neptunian/dc0f75a4e4e91c2029fb34d7c269648d

This will also resolve issue #20

[smeijer]

If you want comments / reviews, isn't it more easy to create a separate branch, and merge it trough a pull-request? Than we can comment on the pull-request instead. It will make both our lives a lot easier. 😄

Happy to help out though.

[neptunian]

@smeijer good idea! Please see the associated pull request! :) @GregoryPotdevin

[neptunian]

I accidentally added an extra commit called "add examples" please ignore and look at this one: 4f2234e

[GregoryPotdevin]

Thanks, easier to read.

It might be a bit hard for the current users to update react-photo-gallery and be forced from a 100% stateful component to a 100% stateless one (ie, pushing the gotoNext functions back into the application).
Maybe have "Gallery" work as it is now, but be a wrapper for a "StatelessGallery" that would be... stateless.

The other thing I find a bit complicated is that you have to pass a lightbox image list ? This could lead to errors if the 2 arrays don't have the photos, or not in the same order (which obviously shouldn't happen, but you never know).
The stateless version in your commit feels like a step in the right direction but in terms of code it almost identical to having something like

<div>
    <Gallery ... />
    <Lightbox... />
</div>

Or maybe it's just a code splitting problem. Keep the Gallery stateful, but extract from it a "GalleryDisplay" (or whatever name fits best). Those using the stateless version would manage the lightbox themselves, or even use a different one.

[neptunian]

@GregoryPotdevin Yes I realize this would be a big change for current users but it's not really that difficult for them copy the methods over right? Keeping the Gallery stateful really limits its potential and I think its more important to have potential than make it easy and limiting.

As for creating a wrapper wouldn't that prevent the user from being able to directly pass in their own Lightbox methods? This is a key feature to me because some of the new features I would like to make capable rely on the user being able to pass in their own methods. How would the user specify routing in this case?

Yes i realize the two image arrays is awkward and more prone to errors but i think there does need to be two separate image arrays. Especially since i want the user to be able to pass custom strings for 'srcset' and 'sizes' (attributes for the img tag) in both the Gallery and Lightbox img elements.

I hadn't thought about having Gallery and Lightbox at the same level or if that would work but I'll play around with that.

[dmr]

@neptunian Version 5.0.0 really is a next step for this component. It was great reading through your commits and how you split out Lightbox step by step, thank you!

[neptunian]

@dmr Thanks! And much thanks to @GregoryPotdevin and @smeijer . Unfortunately I don't get to work with React much in my day to day so I always welcome any advice :)