Ecodev / natural-gallery-js

A lazy load, infinite scroll and natural layout list gallery
https://ecodev.github.io/natural-gallery-js/
MIT License
134 stars 26 forks source link

migration to PhotoSwipe v5 #78

Closed kfsz closed 2 years ago

kfsz commented 2 years ago

Hey, so I tried to get this library working with PhotoSwipe v5 and this is what I ended up with.

Everything seems to work (and work great), but:

Also PS v4 -> PS v5 had some big breaking changes, so here's a short migration guide:

  1. remove photoswipeRef from natural-gallery-js init, it's no longer needed anywhere
  2. neither is photoswipe/lightbox html template, so remove that also (https://ecodev.github.io/natural-gallery-js/docs-options.html#lightbox)
  3. check which options are available in PS v5 (https://photoswipe.com/options/)

after that PhotoSwipe should load ^^

Here are changes I made to API:

  1. added PS getter, so you can now access PS with naturalGalleryInstance.photoSwipe (which can be then used to listen to events, add UI buttons and other things)
  2. also added current PS item getter (naturalGalleryInstance.photoSwipeCurrentItem)
  3. removed zoom event dispatch as it seemed redundant:

Gonna add few comments to files changed to explain why I made some internal changes.
And I think that should be all - please take a look and check if it'd be okay to merge.

resolves #75

sambaptista commented 2 years ago

Thanks for you contribution, it's very appreciated. I will take a while to review your job and validated it.

sambaptista commented 2 years ago

I use the docs as dev support with yarn dev + 127.0.0.1:4000. When I run your fork, I get an error :

capture 2022-05-24 at 20 21 46

I haven't made any updates to docs - I could try updating them if you'd like me to though

Is the error due to this ?

If yes, I would appreciate to get at least the 127.0.0.1:4000 homepage running with the new version of the lib.

kfsz commented 2 years ago

@sambaptista

yeah, I completely skipped homepage last time. fixed it today (and lots of other issues I noticed along the way :stuck_out_tongue: ) - should work as expected now.

with exception of captions - they are no longer supported out of the box (https://photoswipe.com/caption/), but it's possible to implement them via API or plugins - I added a quick and dirty example how to do that for Natural Gallery (index.html), skipping others, as I can't decide if it's better with or without them - I think I'd skip them on main gallery pages and maybe add an example on how to use them in Theming and customisation, not sure though.

also, still saving documentation part for later, but besides that, everything should work, I think.

sambaptista commented 2 years ago

OK; It's finally done, I've reviewed the code and tested it. Thanks a lot for your contribution.

Almost everything is ok. I will do a few adjustments on my side and merge your two pull requests.

Would you have time to fix the tests ? E2E are ok, but UNIT fail. The issue is about the import below.

yarn test unit
    Cannot find module 'photoswipe/lightbox' from 'src/js/galleries/AbstractGallery.ts'

Keep me informed about your intentions to fix it (or not).

PowerKiKi commented 2 years ago

@kfsz, if you push new commits to fix tests, then the CI should trigger and validate your changes. If it doesn't trigger, please let us know and we'll investigate why it didn't trigger.

kfsz commented 2 years ago

@sambaptista @PowerKiKi

tests fixed, they should work (and pass) now.

workflow needs approval though, so please approve.

PowerKiKi commented 2 years ago

Thanks, I approved the workflow, and it did pass 🎉

I'll let @sambaptista do the merging.