Feature: swipe ui

[jossmac]

Description of changes:
Attempt to integrate feature/swipe-support into new fullscreen layout

Related issues (if any):

1. Consider adding touch support (Consider adding touch support #15)
2. Swipe with transition (Swipe with transition #99)

Checks:

• Please confirm npm run lint ran successfully
• Please confirm that only /src and /examples/src are committed
• [if new feature] Please confirm that documentation was added to the README.md

[jossmac]

Hi @jkettmann,

We've both made pretty dramatic changes to the codebase. This branch feature/swipe-ui is my attempt to merge swipe-support and fullscreen-ui unsuccessfully.

Is there any chance you could help?

[jkettmann]

Hi @jossmac, of course. I'll have some time the next days. I'll keep you posted. Any particular area you need help with?

[jossmac]

@jkettmann that's terrific, thank you!

I'm struggling a bit with the new <SwipeContainer /> component - I haven't implemented anything using react-motion, so it's unfamiliar territory. There's also some decisions that I don't understand, like providing the source 'data:' when an image is not visible?

Basically if you could just take a look I suspect you'll notice something obvious that I've missed whilst integrating your changes.

[jkettmann]

I made my changes now and pushed to my forked feature/swipe-ui branch. I'm not sure how the workflow is continuing now. Should I just create a pull request from my forked branch to feature/swipe-ui?

Anyways:
LightBox now only uses SwipeContainer, so we also have horizontal animation on non-mobile devices. The name SwipeContainer seems a bit confusing in this way.

I'm not totally sure why setting source to something for non-visible images was important, but data: seems to be wrong. As far as I remember I did that because of performance issues: SwipeContainer is a wrapper around all images, so it's windowWidth * imageCount wide. If all image sources would be set, this would cause memory problems when there are many and/or large images. So we need to unset sources of non-visible images. One way of doing that is like in this answer.

On mobile devices there is a small issue now, because the user cannot close the LightBox if he is not active anymore.

[jossmac]

Thank you @jkettmann, I appreciate you diving back into this

1. Yes, please create a new pull request from your branch to feature/swipe-ui
2. In my manual merge attempt I had intentionally forked the behaviour of touch vs desktop devices. I feel that having the image replaced in situ (with the loading indicator when applicable) feels more responsive on desktop. Are you cool with that?
3. The solution on StackOverflow seems fine, though perhaps I'm missing the problem. Can't we request the image (or two) ahead of the direction the user is headed rather than load them all at once? If so, there's already a method on Lightbox in the fullscreen branch.
4. I'll look into the userIsActive issue when this is merged; should be fine with the forked behaviour mentioned above

[jkettmann]

Hi @jossmac

1. done
2. that's fine by me. I just like to have an animation on desktop too. Otherwise it looks kind of static to me. I removed Desktop behaviour in the second commit I guess
3. Yes, actually I remember seeing that. I'm not sure, if there is any reason to use my implementation as well. I'll look into that.

[stefano-DM]

when will this be added to master? thanks

[neptunian]

if @jkettmann could fix the conflicts i can look at merging it, otherwise i'll look at it when i can.

[jkettmann]

@neptunian I'll probably have some time at the weekend

[jkettmann]

Hi, just a small update: I didn't find enough time on the weekend. I'll let you know soon

[neptunian]

@jkettmann is this merge for two different features? full screen and swipe or does full screen already exist? i don't see anything about that in the docs.

[jkettmann]

@neptunian It's been quite some time, but as far as I remember it was something @jossmac was working on and we merged the two behaviors into this branch. But I'm not really sure

[neptunian]

@jkettmann do you recall the reason for that? otherwise it seems like it'd be easier to separate these features out for simplicity's sake.

[jkettmann]

@neptunian No I don't remember a reason. You're right, it would be easier to separate them.

[jkettmann]

@neptunian I resolved the conflicts. Should I create a pull request to feature/swipe-ui or somewhere else?

[neptunian]

@jkettmann i think we should probably keep it on this branch instead of starting over and separating things. perhaps @jossmac will be able to pick up where he left off again. Did you ever look into #3 that you mentioned above with loading of the images one or two ahead instead of loading them all?

[jkettmann]

@neptunian Sorry it took me so long. I created a new pull request, because this branch is already quite messed up. You would have to resolve the conflicts yourself. If you would like me to change something just tell me.

[ooloth]

Would love to use this component as soon as touch support is added!

[michalica]

I would like to use swipe function in current version ... is it gonna be possible?

[neptunian]

addressed in #199