Skip to content
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

Open
m00s opened this issue May 4, 2016 · 14 comments
Open

Animation support for multiple galleries #10

m00s opened this issue May 4, 2016 · 14 comments

Comments

@m00s
Copy link
Owner

m00s commented May 4, 2016

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
Copy link

hirbod commented May 4, 2016

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 m00s changed the title Support multiple galleries Animation support for multiple galleries May 5, 2016
@m00s
Copy link
Owner Author

m00s commented May 5, 2016

@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
Copy link

hirbod commented May 5, 2016

@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
Copy link
Owner Author

m00s commented May 5, 2016

@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
Copy link

hirbod commented May 5, 2016

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

@m00s m00s closed this as completed in 942b426 May 5, 2016
@hirbod
Copy link

hirbod commented May 6, 2016

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
Copy link

hirbod commented May 6, 2016

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

@hirbod
Copy link

hirbod commented May 6, 2016

Well, this PR is totally broken.

  • If I create multiple ng-photoswipes per ng-repeat -> gallery broken
  • if I just create one as I had before, the gallery works as intended, but animation will also just work for the last one, even If I can see that the gid=myid is added to the url. So my purpose, adding opts.galleryUID inside of my showGallery function works.

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
Copy link
Owner Author

m00s commented May 6, 2016

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.

@m00s m00s reopened this May 6, 2016
@hirbod
Copy link

hirbod commented May 6, 2016

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
Copy link

hirbod commented May 6, 2016

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
Copy link

hirbod commented May 6, 2016

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
Copy link
Owner Author

m00s commented May 6, 2016

@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
Copy link

hirbod commented May 6, 2016

👍 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.

Repository owner deleted a comment Mar 4, 2024
Repository owner deleted a comment from jacky-xbb Mar 4, 2024
Repository owner deleted a comment from jalamari2018 Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants