-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Animation support for multiple galleries #10
Comments
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 |
@hirbod Are you able to set a different selector (class/id or whatever) to different posts? |
@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(.)? |
@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 |
Alright, good job. Will be happy to incorporate your edits. Thank you very much! |
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) |
Btw, you need to documentate the Gallery UID in your Readme, as nobody will know how to use it currently |
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. |
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. Also, about the galleryUID, there is a note in the README:
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: |
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 ;) |
Also, about the galleryUID, there is a note in the README:
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. |
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: ... |
@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.. |
👍 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. |
From #9 by @hirbod
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
The text was updated successfully, but these errors were encountered: