dimsemenov / PhotoSwipe

JavaScript image gallery for mobile and desktop, modular, framework independent
http://photoswipe.com
MIT License
24.23k stars 3.31k forks source link

Transitions between slides / Animate on arrow right/left click ? #660

Open benbonnet opened 9 years ago

benbonnet commented 9 years ago

Read though the doc, not an option ? That'd make it 200% perfect to make it possible to animate right/left, or maybe fading the images, when the arrows are clicks

benbonnet commented 9 years ago

@dimsemenov think it is somehow possible ?

dimsemenov commented 9 years ago

Transition between items is removed on purpose. Reasons:

  1. On slow machines transitions do not look smooth, especially if large images are used and during the loading of images.
  2. As progressive loading of images is enabled, it causes Paint each time some part loads. If animation will start before some image is fully loaded, there will be lags. Again, especially if images are big (1200px+).
  3. Transition (especially "move") distracts from the content. Most native image viewers don't use any transition. As well as web photo-giants, like 500px, Flickr or Imgur.

In summary, there are no plans to add "move" transition between slides. "fade" - likely will be implemeneted, but currently there are more important features in todo list.

So you'll need to implement it by yourself, for example look in https://github.com/dimsemenov/PhotoSwipe/blob/master/src/js/down-move-up-handlers.js#L1002 (function _finishSwipeMainScrollGesture). Feel free to submit PR, if it'll be light, I'll accept it.

benbonnet commented 9 years ago

Thanks a ot for your answer, I'll dig into it Very clean work btw

petercarsater commented 9 years ago

I just took a quick look at it. When images are changed, the .pswp__container is using translate3d to change the position.

So, correct me if i wrong, a solution would be to add:

.pswp__container { transition: transform 0.35s ease-in-out; }

If you use modernizr you could add (to make sure of support on devices with no touch)

.csstransforms3d.no-touch .pswp__container { ... }

Since it's already using GPU you should have no repaints, and a nice smooth transition between images? Since i haven't test it, i'm not sure about the lag mention above (progressive loading)...

benbonnet commented 9 years ago

What about the growing image animation provided in the demos ? Not that easy to integrate.

As we're not photo-giants yet, you'd get the same fancy appeal with those transitions options, out of the box for everyone

csborgman commented 9 years ago

As a pro photographer for over 20 yrs having a smooth transition between images is really needed. I have to look at things in the eyes of an art buyer, they are looking at hundreds of images each day and the better they UX is the more images they will want to look at.

Comparing a website with photography as the main or only content on a page to giant websites like 500px, Flickr or Imgur is not accurate. Those sites are loading tons of information on each click, my website only goes to the next image, a big big difference.

I am very happy and thankful you've chosen to take over PS, it's never really worked well on Android till now. The comments I have are a suggestion from a pro shooter who like to build website, not as a coder. I can say with 100% confidence adding a slide animation would take it to the next level (only for those using it on desktop, mbl is perfect as is). If slide is not possible then a dissolve into the next image, or the least impressive but still better than no transition is a fade in/out.

Same goes for Magnific Popup, needs a smoother UX for pro or serious enthusiast photographers.

Larzans commented 9 years ago

I do get that it can be a performance problem on some slow machines, but what is the problem with adding an option to use transitions? (which apparently already existed before but was removed : https://github.com/dimsemenov/PhotoSwipe/issues/519#issuecomment-70091664

btw, the link mentioned above does not work anymore, any idea where to find an example on how to best implement this feature with photoswipe?

mjau-mjau commented 9 years ago

+1

A transition option would add a level of satisfaction to the UI for capable desktops (90% +). Just because FB/Flickr/500px don't use animations (for many reasons), doesn't mean it has no functional benefit when added with tasteful easing and interval. Just like photoswipe uses animations for image-zoom and swipe, slide transitions can compliment the UI by visually connecting two states for non-touch devices also.

It would require a truly ancient device to not be able to perform a translateX transition effectively for a single image. As for the issue with progressive images, perhaps one could use a flag to only allow transition if image==loaded?

This seems like a missing ingredient in an otherwise tasty cake. I will probably take a look into a "fix" myself as soon as I find the time, unless it somehow gets added in the meantime.

Larzans commented 9 years ago

I am lost, does anybody know how to implement this with photoswipe? Any help would be appreciated...

benbonnet commented 9 years ago

@larzans this discussion asks about it

Larzans commented 9 years ago

This here is just about the OPTION to do it, what i meant was how to implement it myself without modifying the library itself... I actually found another comment here (https://github.com/dimsemenov/PhotoSwipe/issues/487) which says he got it to work for one image, will try it now...

benbonnet commented 9 years ago

is it surely not impossible to do it yourself @dimsemenov you'll make twice the moneyfor the wordpress plugin on themeforrest with a transition ! Or is it in the paid version ?

dimsemenov commented 9 years ago

I already explained in the first message why there won't be "move" transition and when "fade" transition will be implemented, please re-read it.

There are a plenty of sliders that allow fancy transitions, PhotoSwipe is not one of them. It's not that easy to animate two 1600px wide images, as some people think in this discussion.

benbonnet commented 9 years ago

Let's close this issue then !

dimsemenov commented 9 years ago

@bbnnt, issue will be closed when fade transition will be implemented.

benbonnet commented 9 years ago

:+1:

csborgman commented 9 years ago

Royal Slider seems to do a very good job at doing a "move" slide transition with 1600px wide images. Maybe the PS4 author should talk to the Royal Slider author and ask how he did it??? ;) LOL

The reason there will never be a move transition in PS or Magnific Popup is because Royal Slider is a premium plugin with these features and the author would rather you buy RS than request a feature for a free plugin. He's a very talented coder and a good businessman. I just wish, especially here on github, he would be honest about his business plans and say "why would I add features to a free plugin that would possible cause me to lose Royal Slider sales?" How could anyone argue with that logic?

My suggestion or hope, would be he builds a premium version of PS4 ( a WP plugin!) that adds both fade and move transitions and the UI is biased towards the desktop, but of course still work on mobile; it is still the best mobile option. Maybe a hybrid of Magnific Popup, PS4 and Royal Slider (without thumbnails options).

If PS4 was like PS3 and really just a mobile option this discussion would not exist. I'm happy this is not the case and we are having this discussion. :)

dimsemenov commented 9 years ago

@csborgman,

Royal Slider seems to do a very good job at doing a "move" slide transition with 1600px wide images.

It doesn't. And RS doesn't force progressive loading and stretched thumbnail, which is a huge factor.

The reason there will never be a move transition in PS or Magnific Popup is because Royal Slider is a premium plugin with these features and the author would rather you buy RS than request a feature for a free plugin.

It's a stupid assumption. These are different scripts for a different purpose. And I never had a goal to skip some feature just to make other script sell better.

My suggestion or hope, would be he builds a premium version of PS4 ( a WP plugin!) that adds both fade and move transitions and the UI is biased towards the desktop,

WP plugin will be free, at least most part.

Please keep the discussion constructive.

benbonnet commented 9 years ago

RS is completely bloated with options. keep it simple, it will be way better like this.

csborgman commented 9 years ago

@dimsemenov Of course RS moves/slides 1600px images, why would you say it doesn't? It may not for you but it works fine on the site I'm testing with 1600-1800 x 1200-1300px images at fullscreen on 27" 2009 iMac.

I'm not a coder, just a professional photographer for over 20 years. If I say it works, it really does. I wouldn't lie... Give RS more love man!

Larzans commented 9 years ago

Did anybody manage to get it to work with whichever js trick applied to photoswipe? I am not talking about an option but about dirty hacking, finding some way to do it...

Larzans commented 9 years ago

If anybody is wondering how to get this working with the current version of photoswipe, check out my SE post here: StackOverflow

mjau-mjau commented 9 years ago

Without debating implementation of this feature, I am sure @dimsemenov has reasons, I would like to explore some technical aspects. Today I poked around a bit, and the simple solution with CSS, although not without flaws, is as @petercarsater suggested: .pswp__container_move { transition: transform 333ms cubic-bezier(0.4, 0, 0.22, 1); }

Apart from performance factors, I can only see a few issues with this implementation:

1. loop seam If looping is enabled, the animation will "jerk" at the seam between the last slide and the first slide as translateX position is reset. Not a catastrophe of course, but looks unprofessional. One solution is to disable looping. However, I see that when swiping, the translateX position never gets reset ... It only resets when using non-swipe navigation. Not resetting the translateX would solve this factor, and I am not 100% sure why reset is applied for non-swipe but not for swipe.

2. transition breaks swipe The transition CSS interferes with swipe/drag functionality. The simple solution would be to disable PS swipe feature on desktop/mouse/no-touch, and only add the transition css class on desktop/mouse/no-touch with css media query or JS. An even better solution would be to temporarily remove the transition class on mousedown/startdrag, and reapply on mouseup or afterChange.

3. Zoom state If image is zoomed, transition will be a bit ugly since the image first jumps to original zoom before transition. I think this is an acceptable imperfection.

I will probably have a go at it tomorrow, but any thoughts are welcome.

mjau-mjau commented 9 years ago

PhotoSwipe Slide Transitions So, I added slide transitions to Photoswipe, and it's working nicely without disturbing native behavior. http://codepen.io/mjau-mjau/full/XbqBbp/ http://codepen.io/mjau-mjau/pen/XbqBbp

The only limitation is that transition will not be applied between seams in loop mode (for example when looping from last slide to slide 1). In examples I have used jQuery.

Essentially, it works by simply adding a CSS transition class to the .pswp__container on demand, but we need to add some javascript events to prevent the transition from interfering with Swipe, and only if mouseUsed. We also add a patch so the transition does not get added between loop seams.

1. Add the below to your CSS It will be applied on-demand from javascript when required.

.pswp__container_transition {
  -webkit-transition: -webkit-transform 333ms cubic-bezier(0.4, 0, 0.22, 1);
  transition: transform 333ms cubic-bezier(0.4, 0, 0.22, 1);
}

2. Add javascript events to assist in assigning the transition class This can go anywhere, but must be triggered after jQuery is loaded.

var mouseUsed = false;
$('body').on('mousedown', '.pswp__scroll-wrap', function(event) {
  // On mousedown, temporarily remove the transition class in preparation for swipe.
  $(this).children('.pswp__container_transition').removeClass('pswp__container_transition');
}).on('mousedown', '.pswp__button--arrow--left, .pswp__button--arrow--right', function(event) {
  // Exlude navigation arrows from the above event.
  event.stopPropagation();
}).on('mousemove.detect', function(event) {
  // Detect mouseUsed before as early as possible to feed PhotoSwipe
  mouseUsed = true;
  $('body').off('mousemove.detect');
});

3. Add beforeChange listener to re-assign transition class on photoswipe init The below needs to be added in your PhotoSwipe init logic.

// Create your photoswipe gallery element as usual
gallery = new PhotoSwipe(pswpElement, PhotoSwipeUI_Default, items, options);

// Transition Manager function (triggers only on mouseUsed)
function transitionManager() {

  // Create var to store slide index
  var currentSlide = options.index;

  // Listen for photoswipe change event to re-apply transition class
  gallery.listen('beforeChange', function() {

    // Only apply transition class if difference between last and next slide is < 2
    // If difference > 1, it means we are at the loop seam.
    var transition = Math.abs(gallery.getCurrentIndex()-currentSlide) < 2;

    // Apply transition class depending on above
    $('.pswp__container').toggleClass('pswp__container_transition', transition);

    // Update currentSlide
    currentSlide = gallery.getCurrentIndex();
  });
}

// Only apply transition manager functionality if mouse
if(mouseUsed) {
  transitionManager();
} else {
  gallery.listen('mouseUsed', function(){
    mouseUsed = true;
    transitionManager();
  });
}

// init your gallery per usual
gallery.init();
Larzans commented 9 years ago

If you have problems with your implementation please check the solution i mentioned in my post above on StackOverflow: http://stackoverflow.com/questions/28214038/photoswipe-4-0-initiate-swipe-to-next-programatically/28320098#28320098 I found no problems looping or any other problem for that matter.

Graham-Openshaw commented 8 years ago

I'm a total JS newbie so this is largely over my head and I'm struggling with where to place the code above but I also have a couple of potentially stupid questions: a) in photoswipe.js line 525 there is a class added (somewhere.....) called pswp--has_mouse. If this literally means what it says could the transition be added to this class or at least by the same event logic? b) it seems that the only time this transition is an issue is when the button--arrow--left/right buttons are used. Could those events be used to trigger the class add from photoswipe-ui-default.js line 461/466?

I tried the straight CSS solution someone suggested but it makes drag events horribly sluggish.

Thanks!

UPDATE: The code above (mjau-mjau) works fine - thanks a lot, it makes the otherwise awesome plugin usable for me.

No need to answer the above questions unless you are bored enough to want to help educate me....

Graham-Openshaw commented 8 years ago

Further update: mm code seems to cause horrible hesitation in IOS Safari so back to vanilla implementation. May try Larzans when I understand it.

mjau-mjau commented 8 years ago

@Graham-Openshaw thats because 'mouseUsed' code is outdated in my example from almost a year ago. 'Mousemove' will in fact trigger on iOS now (although not continuously), so this causes crash of swipe and transition, thus creating a sluggish motion. You need to use a more advanced detection method. I have an updated version. You can see it works fine here, also on iOS Safari: https://demo.flamepix.com/galleries/nature/

Graham-Openshaw commented 8 years ago

Thanks! That makes sense. Extracting the revised code from the demo is a bit beyond my very limited capability and I have SO much to learn.... If you ever put the updated version in a codepen or similar please let me know.

mjau-mjau commented 8 years ago

Actually, if you just remove my native mouseUsed detection, it should work. The native mouse detection in Photoswipe is almost identical to my new code, and checks that mousemove triggers multiple times within 100ms, thus to be sure it's not a pseudo mobile event. The only reason I am using separate mouse detection in the first place, is because photoswipe doesn't start detecting mousemove until after it gets triggered. This could mean that transitions don't fire on first slide, or until visitor moves mouse again ...

Try this. Same scripts as posted earlier, but remove the commented stuff:

var mouseUsed = false;
$('body').on('mousedown', '.pswp__scroll-wrap', function(event) {
  // On mousedown, temporarily remove the transition class in preparation for swipe.
  $(this).children('.pswp__container_transition').removeClass('pswp__container_transition');
}).on('mousedown', '.pswp__button--arrow--left, .pswp__button--arrow--right', function(event) {
  // Exlude navigation arrows from the above event.
  event.stopPropagation();
})/* Don't use below any more, because it may detect 'mouse' on mobile devices
.on('mousemove.detect', function(event) {
  // Detect mouseUsed before as early as possible to feed PhotoSwipe
  mouseUsed = true;
  $('body').off('mousemove.detect');
});*/
// downgrade to ONLY use the photoswipe listen event for mouse movement
/*if(mouseUsed) {
  transitionManager();
} else {*/
  gallery.listen('mouseUsed', function(){
    mouseUsed = true;
    transitionManager();
  });
//}
Graham-Openshaw commented 8 years ago

Thanks Karl! That works great. Here is the result: All I need to do now is learn enough to stop 'borrowing' code and write my own - or at least understand the code I'm borrowing.......

mjau-mjau commented 8 years ago

Thanks Karl! That works great. Here is the result:

Wow, looks smooth! Did you change the CSS transition?

isapir commented 8 years ago

@petercarsater .pswp__container { transition: transform 0.35s ease-in-out; } seems to work beautifully on my desktop. Is there any downside to that?

Actually, I see that on a mobile device it makes the original transition a bit laggy. Maybe it should be added for non-touch devices only or something like that, or be removed when swipe is used.

mjau-mjau commented 8 years ago

@TwentyOneSolutions Downside is that it messes up drag/swipe behavior. Try dragging the image on desktop, or swiping from a mobile device, with that CSS transition included.

That's why the transition CSS needs to be toggled based on interaction event. See my earlier posts.

isapir commented 8 years ago

@mjau-mjau You're right, I noticed that just after I posted my message and updated my comment accordingly.

I saw your solution, but I'm not sure that I like listening to mouse-down event anywhere on the page just for that. I was thinking about a different solution:

function slide(direction, duration) {

    if (direction != "prev")
        direction = "next";     // default

    duration = duration || 350;

    // inject style
    $('<style id="pswp-style-injected" type="text/css">.pswp__container { transition: transform ' + duration + 'ms ease-in-out; }</style>')
        .appendTo("head");

    photoSwipe[direction]();

    setTimeout(
        function(){
            // remove the injected style
            $("#pswp-style-injected").remove();
        }
        ,duration
    )
}

This assumes a global variable named photoSwipe which points to the PhotoSwipe object.

Then you can call slide("next") or slide("prev") and it slides with ease-in-out transition, so it's easy to wire the Left/Right arrows and Left/Right keys to slide() instead of the current handler, without touching the swipe/drag events.

Feedback welcomed.

mjau-mjau commented 8 years ago

I saw your solution, but I'm not sure that I like listening to mouse-down event anywhere on the page just for that.

Err, why not? It's a mouse-down event bound to the specified elements. It's the most effective way of applying a click-event on the arrow items (or any other elements). How were you thinking of applying your slide code without doing it by 'on click'? http://api.jquery.com/on/

Perfectly legitimate code, and works nicely as you can witness on all devices here: https://demo.flamepix.com/galleries/landscapes/

I don't think injecting a stylesheet into root will get computed the same nanosecond as user clicks "next", and it's certainly not good performance or practice to toggle a stylesheet just for an operation. The difference between my code and yours, is that you are 1) injecting a stylesheet, instead of simply toggling a class, and 2) disabling the transition based on an inaccurate timeout, instead of a fully legitimate event that knows when it's safe to toggle the transition. 3) Your code requires adding your own arrow buttons ... How were you thinking of disabling the native photoswipe buttons? My code doesn't interfere with default photoswipe behavior. Oh ... and you would need to disable PS-keyboard events also, and add your own. 4) Of course, your code will mess up if the visitor happens to drag/swipe within duration-time, right after your 'slide' function is triggered.

isapir commented 8 years ago

Many good questions. I actually asked @dimsemenov about it at https://github.com/dimsemenov/PhotoSwipe/issues/1012

mjau-mjau commented 8 years ago

You are chasing a dead-end, and an inferior solution ... Especially considering your comment about mousedown. You are aware that when adding ANY click event from JS to a button, including yours, it is most effectively done like this:

$('body').on('click', 'SELECTOR' ...

The above does not in any way insinuate a click "anywhere on page" ... It triggers on the SELECTOR.

isapir commented 8 years ago

You are aware that when adding ANY click event from JS to a button, including yours, it is most effectively done like this: $('body').on('click', 'SELECTOR' ...

No, I'm not aware of that. Are you sure? I tried to look it up but I can't find any documentation that supports that statement.

$('body') does listen for a click anywhere on the page, then filters it according to the selector. It is possible that the internals of jQuery use that same technique for $(selector).on()..., i.e. listening on the body element and filtering by the selector, but I'm not aware of that.

mjau-mjau commented 8 years ago

Event delegation has two main benefits. First, it allows us to bind fewer event handlers than we'd have to bind if we were listening to clicks on individual elements, which can be a big performance gain. Second, it allows us to bind to parent elements — such as an unordered list — and know that our event handlers will fire as expected even if the contents of that parent element change.

Your browser still needs to process a click event, no matter where it is on page. If its an event delegated to a button, it obviously ONLY triggers when you click the button. Besides, this is a CLICK event, not scroll event that triggers 100s of times a second. http://stackoverflow.com/questions/14618435/are-there-advantages-or-disadvantages-to-binding-on-a-parent-element-versus-the https://learn.jquery.com/events/event-delegation/ http://stackoverflow.com/questions/8110934/direct-vs-delegated-jquery-on

Of course, add the bonus that a delegated event only needs to be assigned once, and does not have to be re-applied each time you for example open photoswipe, and also remains persistent on ajax pages.

If you REALLY don't like delegated events, like the following: $('body').on('mousedown', '.pswp__scroll-wrap', function(event) { ...

Then just assign the event as non-delegated ... simple: $('.pswp__scroll-wrap').on('mousedown', function(event) {

The two functions above will do the same thing, and you won't be able to detect on performance-difference between them. Only difference is that the bottom would need to be re-applied on the element each time it is created.

Besides, if you are concerned about performance ... A delegated click event is certainly much much faster than injecting a stylesheet on click, which would require the browser to scan through the entire DOM.

isapir commented 8 years ago

OK, here's my solution:

1) I created a simple jQuery plugin that fires an event whenever Drag begins or Ends, you can find it at https://gist.github.com/TwentyOneSolutions/c82166d0b49dc5237759978553c4d158

2) I wrote two helper functions to set and clear the transition:

function clearTransition() {
    $(".pswp__container").css({ "transition": "" });
}

function setTransition() {
    $(".pswp__container").css({ "transition": "transform 0.35s ease-in-out 0s" });
}

3) I call setTransition() when an arrow key is pressed, or when drag ends, and clearTransition() when drag starts, courtesy of the aforementioned plugin and its drag:changed event:

$(function(){

    // set transition on arrow keys down
    $(".pswp").on("keydown", function(evt){
        if (evt.which == 37 || evt.which == 39)
            setTransition();
    });

    // listen to dragchanged events and set transion on drag end
    $(".pswp__container")
        .on("drag:changed", function(evt, isDragging){
            if (isDragging)
                clearTransition();
            else
                setTimeout(setTransition, 500);
        })
        .dragchanged(); // wire plugin so the events will fire
});

4) After creating a PhotoSwipe object, I listen to the mouseUsed event and call setTransition() from it, so that transition is used when the UI Navigation buttons are clicked:

photoSwipe.listen("mouseUsed", function() {
    setTransition();        // set transition to capture nav buton clicks
});

You can see a working example at http://www.intelliflame.com/gallery.htm

mjau-mjau commented 8 years ago

It almost works properly ... It messes up transition when one drags next slide before the 500ms timeout is done, and the transition in the loop seam is a bit strange. Sorry, but not sure why you are re-inventing a solution that is less precise, requires more code, and does not work flawlessly.

isapir commented 8 years ago

It almost works properly ... It messes up transition when one drags next slide before the 500ms timeout is done

No, it actually does work properly. The issue that you reference is due to a different hack, which allows to use dynamic sizes for images rather than specifying the data-size, as I documented in #741

As for the non-transition in the loop seam -- your code suffers from the same issue so I really am starting to think that you are trying really hard to "find" issues with my code, which works very well for me, and I posted it above for the benefit of others.

mjau-mjau commented 8 years ago

No, it actually does work properly. The issue that you reference is due to a different hack, which allows to use dynamic sizes for images rather than specifying the data-size, as I documented in #741

Where do you get this from? Dynamic sizes should get stored if done properly, so the error would not re-occur after the images have been seen once.

As for the non-transition in the loop seam -- your code suffers from the same issue so I really am starting to think that you are trying really hard to "find" issues with my code, which works very well for me, and I posted it above for the benefit of others.

No. My code has specific implementation, that simply disables the transition when looping between first and last item. The transition is therefore instantaneous. This is clearly evident when comparing mine vs yours. See code below.

// Only apply transition class if difference between last and next slide is < 2
// If difference > 1, it means we are at the loop seam.
var transition = Math.abs(gallery.getCurrentIndex()-currentSlide) < 2;

I really am starting to think that you are trying really hard to "find" issues with my code, which works very well for me, and I posted it above for the benefit of others.

As I said, your code almost works fine. If it was better, I would happily say so. Unfortunately, your code has some flaws ... and according to you, the only flaw in my code is that Im assigning a click event delegated to buttons (huh).

isapir commented 8 years ago

The issue that you were seeing is likely (I say likely because I'm not looking at your screen so it's hard to tell what you are seeing exactly) due to a bug in PhotoSwipe. I issued a patch at #1178

Anyway, if you're happy with your solution -- great! I posted mine for the benefit of others who might look for a different solution.

isapir commented 8 years ago

@dimsemenov The best way to handle the transition would be with the same logic that you wrote at https://github.com/dimsemenov/PhotoSwipe/blob/master/src/js/gestures.js#L1001

The animation works very well, and the transition between last-to-first and first-to-last works properly there (in drag/swipe), but it doesn't work with transitions alone.

Is it possible to extract the logic of the code there to an external function, and then call it from next() and prev()? That would be the best and cleanest solution IMO.

Thanks!

mjau-mjau commented 8 years ago

The animation works very well, and the transition between last-to-first and first-to-last works properly there (in drag/swipe), but it doesn't work with transitions alone.

That is because the logic of PS swipe is to continue moving the stage on the X plane, also after the loop seam. On non-swipe however, the stage returns to original 0 position after the loop.

That is why in your version, you are seeing a strange "delay" in the seam between the loop, which ultimately you should avoid by disabling transition in the loop seam. Dimensov will not change the logic of this for your "fix", as it is part of the PS core ... only if he officially added support for transitions perhaps.

The issue that you were seeing is likely (I say likely because I'm not looking at your screen so it's hard to tell what you are seeing exactly) due to a bug in PhotoSwipe. I issued a patch at #1178

The issue can easily be explained, because you are using a timeout instead of a true event to toggle the transition. This means, that if I drag faster than your timeout, double-transitions will occur, corrupting the transition: https://youtu.be/KA3oYWZmViE

If you toggle the transition based on true events, like in my version, this is avoided: https://youtu.be/dgLl_roNXMM

I'm still not sure why you are spending resources on presenting a less precise solution. What was wrong with my offer? Im not trying to "pick" on your code, but you are specifically asking for feedback in this post from users with experience, and this is it.

isapir commented 8 years ago

@mjau-mjau I build applications for normal users. Normal users do not drag with the mouse at the pace of 3 times a second. I just visited your demo links again and when I put them to the same non-real-world test of dragging very fast they are jerky and inconsistent.

You wrote those lengthy messages and even did video captures and you're asking me why I waste my time? I think that you are the one who's wasting time, and each time you post here about it everyone on the thread receives an email.

If you have something constructive to say -- great, but please do not post more messages about how your solution is better than mine, because that is your opinion, and it may be biased.

I like my solution better. As I wrote before -- if you are happy with your solution then that's great -- I'm happy for you!

mjau-mjau commented 8 years ago

I just visited your demo links again and when I put them to the same non-real-world test of dragging very fast they are jerky and inconsistent.

Well, it's the exact same CSS transition going on with mine and yours. The only difference is that my images are larger than yours, thus the transition may seem slightly less smooth. Image size is of course arbitrary, and not something in the code.

The issues in question, are the important parts of the code that trigger the events and the actual functionality of the transitions.

If you have something constructive to say -- great, but please do not post more messages about how your solution is better than mine, because that is your opinion, and it may be biased.

I am the only one answering all your questions, truthfully, and how am I NOT being constructive? Unlike you, I have posted technical explanations in ALL my replies, directly related to what you are asking. Nobody else will answer you any more constructive. How are your posts constructive btw?

I don't have time to sugar-coat my replies for you if that's what you want. I said it before, and I will say it again: Your solution works ok, but it has some minor flaws ... which would "acceptable" if the code was more lightweight than solutions posted earlier, but it's not.

dimsemenov commented 8 years ago

@TwentyOneSolutions, please keep the discussion on topic, @mjau-mjau is just trying to help you.

@dimsemenov The best way to handle the transition would be with the same logic that you wrote at https://github.com/dimsemenov/PhotoSwipe/blob/master/src/js/gestures.js#L1001 Is it possible to extract the logic of the code there to an external function, and then call it from next() and prev()? That would be the best and cleanest solution IMO.

For now, there is no public API for this feature. It's possible only by writing custom module and animating X position using _animateProp method (the same way as it's done in gestures.js).