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

Added Video Support #137

Open
wants to merge 30 commits into
base: dev
Choose a base branch
from
Open

Added Video Support #137

wants to merge 30 commits into from

Conversation

anjnkmr
Copy link

@anjnkmr anjnkmr commented Jan 2, 2017

I used this script in our site, and we need a requirement of video on popup, so i modified the code the add video support

@feimosi
Copy link
Owner

feimosi commented Jan 2, 2017

Thanks for your contribution, that looks like quite a lot of new code. I'm gonna need more time to review it.
Could you please remove dist/ folder from this PR? I take care of releases myself. Also there seem to be linting errors.

@anjnkmr
Copy link
Author

anjnkmr commented Jan 3, 2017

Remove unmodified files from pull request

@feimosi
Copy link
Owner

feimosi commented Jan 3, 2017

Sorry, you must have misunderstood me. I meant remove dist/ folder changes in this PR, not the whole folder itself.

Just leave src/ changes which are relevant to me. I update dist/ and demo/ myself. Thanks.

@anjnkmr
Copy link
Author

anjnkmr commented Jan 3, 2017

Hi @feimosi please check now
I see something failed here

"continuous-integration/travis-ci/pr",

i don't understand what is this

@feimosi
Copy link
Owner

feimosi commented Jan 3, 2017

Seems fine now. Just unnecessary empty first line in .editorconfig and demo/index.html

I don't understand what is this

It's linter errors. You can see more details here:
https://travis-ci.org/feimosi/baguetteBox.js/jobs/188491776#L188

@anjnkmr
Copy link
Author

anjnkmr commented Jan 4, 2017

I fixed the warnings and removed first empty line in .editorconfig file,
thanks for explanation
and this is my first contribution in github

@feimosi
Copy link
Owner

feimosi commented Jan 4, 2017

Great, I'll take a look in some free time.
No problem if it's your first contribution. We're here to help :)

Also, you may want to read about git commit --amend, so you don't create so many commits.

@anjnkmr
Copy link
Author

anjnkmr commented Jan 5, 2017 via email

demo/index.html Outdated
@@ -1,3 +1,4 @@

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be here.

function loadImage(index, callback) {
var imageContainer = imagesElements[index];
var galleryItem = currentGallery[index];
var isVideo = false;
if(imageContainer !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space/ Also better use typeof imageContainer !== 'undefined'

@@ -490,11 +515,23 @@

// Get element reference, optional caption and source path
var imageElement = galleryItem.imageElement;
var thumbnailElement = imageElement.getElementsByTagName('img')[0];
var thumbnailElement = null;
if(isVideo) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing space afyer if.

var thumbnailElement = null;
if(isVideo) {
thumbnailElement = imageElement.getElementsByTagName('video')[0];
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else should be on the same line.

@@ -477,7 +477,7 @@

function pauseAnyVideoPlaying(){
[].forEach.call(imagesElements, function(imageElement) {
if(imageElement.getElementsByTagName('video').length > 0){
if (imageElement.getElementsByTagName('video').length > 0){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before brackets.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi, do you want me to remove the spaces before brackets

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you should always use it as per the rest of the codebase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fixing

isVideo = videoRegex.test(galleryItem.imageElement.href);
}

// Return if the index exceeds prepared images in the overlay
// or if the current gallery has been changed / closed
if (imageContainer === undefined || galleryItem === undefined) {
if (typeof imageContainer === 'undefined'|| typeof galleryItem === 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space before ||

thumbnailElement = imageElement.getElementsByTagName('img')[0];
}
var imageCaption = typeof options.captions === 'function' ?
options.captions.call(currentGallery, imageElement) :
imageElement.getAttribute('data-caption') || imageElement.title;
var imageSrc = null;
if(isVideo) {
if (isVideo) {
imageSrc = getVideoSrc(imageElement);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use else in the same line as the brackets.

function loadImage(index, callback) {
var imageContainer = imagesElements[index];
var galleryItem = currentGallery[index];
var isVideo = false;
if typeof imageContainer !== 'undefined' {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong now; you are missing the parentheses.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing

@@ -490,11 +515,21 @@

// Get element reference, optional caption and source path
var imageElement = galleryItem.imageElement;
var thumbnailElement = imageElement.getElementsByTagName('img')[0];
var thumbnailElement = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could change this whole thing into a one liner

var thumbnailElement = isvideo ? imageElement.getElementsByTagName('video')[0] : imageElement.getElementsByTagName('img')[0];

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you fixing

var imageCaption = typeof options.captions === 'function' ?
options.captions.call(currentGallery, imageElement) :
imageElement.getAttribute('data-caption') || imageElement.title;
var imageSrc = getImageSrc(imageElement);
var imageSrc = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

// If video is already loaded run callback and return
if (imageContainer.getElementsByTagName('video')[0] && isVideo) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please explain what is problem here, i don't understand

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can merge this condition with the previous one instead of duplicating.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks merged

var imageSrc = getImageSrc(imageElement);

var imageSrc = isVideo ? getVideoSrc(imageElement) : getImageSrc(imageElement);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing spaces.

// If image is already loaded run callback and return
if (imageContainer.getElementsByTagName('img')[0]) {
// If image is already loaded run callback and return OR If video is already loaded run callback and return
if ((imageContainer.getElementsByTagName('img')[0] && !isVideo) || (imageContainer.getElementsByTagName('video')[0] && isVideo)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the && !isVideo needed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is needed

@rafaberaldo
Copy link

Any update on this @feimosi ?

@feimosi
Copy link
Owner

feimosi commented Sep 16, 2017

@rafaelpimpa Sorry, it needs more time from my side because it introduces too many changes and I don't have much time lately.

@elber-rodrigues04
Copy link

Hi feimosi my name is Elber and I'm a Baguette.box's maintainer in Drupal.org (please see it: https://www.drupal.org/project/baguettebox) we have an issue with the same target of this pull request (please see it: https://www.drupal.org/project/baguettebox/issues/3261916) for keep working on that issue I need you commit this changes here.

Please don't forget of this comment.

Thank you all.

@smustgrave
Copy link

Wonder if this is still being used on 1.11.1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants