m00s / angular-photoswipe

An angular directive for PhotoSwipe image gallery
MIT License
73 stars 42 forks source link

Animation support for multiple galleries #10

Open m00s opened 8 years ago

m00s commented 8 years ago

From #9 by @Hirbod

While this is working fine when you only have one gallery, how to go on with multiple ones?

There is an official doc about that: http://photoswipe.com/documentation/getting-started.html#dom-to-slide-objects

and a working example: http://codepen.io/dimsemenov/pen/ZYbPJM

But I really don't have a clue right now, how to use this with this directive. Any guesses?

I think it can be a major improvements even though a not-so-common scenario.

I open this issue to think and discuss about it

hirbod commented 8 years ago

I have multiple galleries already working with your directive, the only problem is animation, as the passed index for the thumbnail-bound-fn will always use the values from the last created gallery (keep in mind, I have a Facebook like newsfeed with multiple posts and every post can have up to 10 images, which are galleries for itsef

m00s commented 8 years ago

@Hirbod Are you able to set a different selector (class/id or whatever) to different posts? In that case you can just pass the right selector to different instances of the directive. (I added a new thumbSelector attribute in the version 0.1.0, not yet released)

hirbod commented 8 years ago

@m00s creating different IDs should work, as I am using ng-repeat. I could just pass $index to an string and use this as ID. Any ETA for your feature? And how do you want to make a difference between id (#) and class(.)?

m00s commented 8 years ago

@Hirbod You'll pass any valid selector. Doesn't matter the query you choose, it will be performed using .querySelectorAll().

I'll provide an example for it in the next release.. think tonight or tomorrow

hirbod commented 8 years ago

Alright, good job. Will be happy to incorporate your edits. Thank you very much!

hirbod commented 8 years ago

Thanks for your changes, but what you've done now will cause a big DOM-Tree, if you're going to do it like I want. Currently, I just have one ng-photoswipe per site, now I have to create multiple for every ng-repeat - or is it possible to change the unique id on-the-fly (e.g. when showGallery is called)

hirbod commented 8 years ago

Btw, you need to documentate the Gallery UID in your Readme, as nobody will know how to use it currently

hirbod commented 8 years ago

Well, this PR is totally broken.

Further of that, since your bump to 0.10 will break the "opts" object, when no getThumbBoundsFn is provided.

  $scope.ps.opts = {
    index: 0,
    history: false,
    showHideOpacity: true,
    shareEl: false
  };

None of these settings gets applied. When I provide getThumbBoundsFn, it works. I guess you're overwriting the opts-object, when getThumbsBoundsFn does not exists. Add to the object, don't override it please.

If I remove the "slide-selector", I will also get an error. (when no bound function was provided)

When I provide getThumbBoundsFn, everything works (except for the animation, even with galleryUID).

For me, this looks totally untested and sadly is not working.

m00s commented 8 years ago

Saw that, I'm sorry for this PR, it hasn't been deeply tested.

However, @Hirbod, instead of raising issues you could just take your time and submit a working PR. As you found a lot of major problems, mostly related to your scenario, you could read those 100 lines of code and help to make them better.

Also, about the galleryUID, there is a note in the README:

provide a parameter to the ng-photoswipe attribute..

there is only one way to do that, and it is slightly different from "Provide an attribute to the ng-photoswipe directive" as you probably understood.

EDIT: Forgot to say, if u are interested to become a collaborator for this repo let me know, I would be more than happy to have you working on this.

hirbod commented 8 years ago

I could start to look at it, but you need to provide the current version as unminified. The unminified is still on 0.9.

Furthermore, I'm more a quality tester, as my use-cases are mostly more advanced. As you may know, time is what always matters. So I could dig into this, or just send you some beer on paypal ;)

hirbod commented 8 years ago

Also, about the galleryUID, there is a note in the README:

provide a parameter to the ng-photoswipe attribute..

Yeah, but think about the unexperienced users. They have to dig into code to understand that. Instead, this should be written down into the Readme, that a opts key with naming "galleryUID" is required. Just saying.

hirbod commented 8 years ago

https://github.com/m00s/angular-photoswipe/blob/master/angular-photoswipe.js#L52-L63

As I thought, you're overwriting the object. First check, if scope.options exists, if not, go on like you did, but if it exists, add it into scope.options.thumb... instead scope.options = thumb: ...

m00s commented 8 years ago

@Hirbod the one I pushed is the 0.1.0, but I didn't update the banner 😞 ..

Yeah, this is what I'm saying.. these are pretty easy and trivial bugs.. the most of the time required is to bundle stuff and publish it..

So I think I can submit a public PR and we can take a look to the code before cutting a new buggy release..

hirbod commented 8 years ago

👍 I will try to help as much as I can. Just to mention, that I'm officially supporting a lot of Open Source Projects, also Cordova Plugins, so my time is limited.