dimsemenov / PhotoSwipe

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

PhotoSwipe with turbolink causes page to refresh after close #700

Open chamnap opened 9 years ago

chamnap commented 9 years ago

Firstly, I would like to say thanks to your hard work on this awesome library.

I'm integrate this library on my rails 4 project with turbolinks. Everything works expected except when I close the popup, the page is refreshed. However, if I take the turbolink library out, it looks fine. I'm not sure what happen, and what I should do. See this, http://www.snapyshop.com/products/54a2f6886c6936668d380000

dimsemenov commented 9 years ago

Hard to tell. Get unminified version of PhotoSwipe and debug what line causes the issue, function returnToOriginal (https://github.com/dimsemenov/PhotoSwipe/blob/master/src/js/history.js#L167) is executed after gallery is closed. Or you can just disable history (history:false).

mitchnick commented 9 years ago

@chamnap Were you able to figure out a solution for this besides removing turbolinks? I have the same issue right now.

inlikealion commented 9 years ago

@chamnap @mitchnick I'm experiencing the same just now. Haven't had a chance to dig in yet (initial experimenting with PS for our app). Have you figured anything out?

abdelbk commented 9 years ago

To whom who still struggle with this issue. I was able to fix it by changing this line : https://github.com/dimsemenov/PhotoSwipe/blob/master/dist/photoswipe.js#L3620

if(_urlChangedOnce) {
    history.back();
}

to :

if(_urlChangedOnce) {
    history.pushState("", document.title,  _windowLoc.pathname + _windowLoc.search );
}

Turbolinks seems to use its own pushState, so it will avoid conflicting with the native one.

caiotarifa commented 9 years ago

@abdelbk :+1:

patricklindsay commented 9 years ago

What is the status of this? I am using the photoswipe-rails gem and have had to deactivate history for now. I could fork the gem but be great to get this fixed in photoswipe as I suspect a lot of people are having this issue.

abdelbk commented 9 years ago

@patricklindsay A pull request is already opened to fix this issue in the photoswipe-rails gem. However, the owner of the gem believes that the fix should be made directly into photoswipe.

patricklindsay commented 9 years ago

Yes I can see that, which is why I'm curious to see if the fix will be made here :)

skakri commented 9 years ago

@patricklindsay, @abdelbk, I doubt that I should introduce any platform-related (think: RoR, Django, etc.) patches/changes in photoswipe-rails (mainly because I'm not sure that I'll be able to keep up with PhotoSwipe changes & manually/semi-automatically patch photoswipe-rails every time), that is all. I just won't have the time to go over every new release.

dimsemenov commented 9 years ago

The reason why I don't want to merge suggested edits by @abdelbk, is that it creates new history record, instead of removing one after gallery is closed. So if user opens gallery, closes it, then again opens and closes, It'll take 5 "back" button clicks to go to the previous page.

Besides that, developer of a site would have to add additional checks to find out if PhotoSwipe URL (&gid=XX&pidXX) was set on purpose to open gallery, or by mistake when going "back".

What might work is changing all occurences of pushState with replaceState in history.js (it would, of couse, break "back button to close" feature). We can add some option in history.js that would control which method should be used.

I'd appreciate if anyone who works a lot with rails would send pull request that adds such option, or I'll implement it by myself. Does Rails have a method that would emulate history.back without breaking anything?

abdelbk commented 8 years ago

@dimsemenov I doubt that. Turbolinks is pretty annoying. As stated in the documentation (https://github.com/rails/turbolinks/blob/master/README.md#events), The back button can't be caught to prevent page reload. I believe that the only current solution is to follow your suggestion and add an additional option.

patricklindsay commented 8 years ago

:+1:

rctneil commented 4 years ago

@dimsemenov Has there been any progress on this? I've just hit this exactly and am unsure what the best option is at this point?

Lordnibbler commented 4 years ago

+1 I am still running into this as well

rctneil commented 4 years ago

I am still hitting this issue. I did post about it in the Turbolinks repo here: https://github.com/turbolinks/turbolinks/issues/549

It sounds like the history entry needs to be in a format that Turbolinks understands (for whatever reason that is) and the example I was shown was:

Turbolinks
    .controller
    .pushHistoryWithLocationAndRestorationIdentifier(path, Turbolinks.uuid())

Anyone able to tweak Photoswipe to allow this method?

vladtabachuk commented 3 years ago
if(_urlChangedOnce) {
    history.pushState("", document.title,  _windowLoc.pathname + _windowLoc.search );
}

This fix is absolutely perfect decision for me!