-
Notifications
You must be signed in to change notification settings - Fork 431
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
# Adds public showImage method #128
Conversation
- adds a public method allowing to open overlay with arbitrary image from galleries - makes data object public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution, really appreciate that.
I'd change the method's name to openImage()
, it's more meaningful.
And I don't understand the last sentence, could you explain it in more detail?
And it works with multiple galleries as well, as I make sure every gallery has it's own ID (either on HTML or created dynamically via javascript) before invoking any image.
Especially thecreated dynamically via javascript
part. I see we always take the first element if multiple galleries have the same selector
function showImage(selector, imageIndex) { | ||
if ( null === imageIndex || undefined === imageIndex || '' === imageIndex ){ | ||
imageIndex = 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd replace this check with:
if (isNaN(parseInt(imageIndex)) {
imageIndex = 0;
}
imageIndex = 0; | ||
} | ||
var activeImage = baguetteBox.data[selector].galleries[0][imageIndex]; | ||
activeImage.eventHandler(activeImage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check that given index doesn't exceed the elements count:
if (activeImage) {
activeImage.eventHandler(new MouseEvent('click'))
}
I've also replaced activeImage
argument, because the method expects an event object.
@@ -692,9 +701,11 @@ | |||
|
|||
return { | |||
run: run, | |||
data: data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we make this data public?
You can refer to data
directly in the code, we're in a closure.
destroy: destroyPlugin, | ||
showNext: showNextImage, | ||
showPrevious: showPreviousImage | ||
showPrevious: showPreviousImage, | ||
showImage: showImage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the trailing comma, it's not supported by IE.
}; | ||
|
||
})); | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing final newline. I recommend using editorconfig.org 👍
Thank you for the great job with BaguetteBox and I am sorry for not contributing more and properly since my health problem. |
No problem, I really hope your health gets better! |
This is an initial effort to create a public open image method for use cases like #127 opened by @Limboistik
Not the best code, but I just wanted to jumpstart the development while sharing the way I solved an issue in 2 of my work projects.
With this code, one can open a image with something like
showImage('#gallery-selector', 0)
.And it works with multiple galleries as well, as I make sure every gallery has it's own ID (either on HTML or created dynamically via javascript) before invoking any image.