1
\$\begingroup\$

I know my way around jQuery and JavaScript a little bit but as for optimising, I'm a bit weak but willing to learn new stuff.

I'm currently loading videos onto our site from Vimeo when a user clicks the appropriate image area. This is working fine, but I feel it's not an entirely performant way to go about doing it.

Does anyone see any issues with the code I've written below which could be done better?

var videoData = [
{
    'player_id':'video1',
    'videoURL':'<?php the_field('two_vimeo_video_url'); ?>',
    'width':'1000',
},
{
    'player_id':'video2',
    'videoURL':'<?php the_field('six_vimeo_video_url'); ?>',
    'width':'1000',
}];

function loadVideo(target, videoid) {
    $.getJSON('http://www.vimeo.com/api/oembed.json?url=' + encodeURIComponent(videoData[videoid]['videoURL']) + '&api=1&player_id='+ videoData[videoid]['player_id'] +'&width='+videoData[videoid]['width']+'&byline=0&color=ffffff&title=0'+'&callback=?', function(data){

        $(target).find('.video-container').html(data.html); // puts an iframe embed from vimeo's json

        $(target).closest('iframe').load(function(){

            $f(player).addEvent('ready', function(id){
                var vimeoVideo = $f(videoid);
            });
        });
    });
}

$(function(){
    // Create array to store values
    var vimeoArray = [];
    var videoContainer = $('.video-container');

    // loop through st
    $(videoContainer).each(function(index, value) {

        // get the image with the data attr on each loop
        var dataAttr = $(this).attr('data-video');

        // if dataAttr is not false
        if ( dataAttr !== undefined) {

            // push data attribute value into array
            vimeoArray.push(dataAttr);

            // Store last element of array on iteration
            var videoid = vimeoArray[vimeoArray.length-1];

            // attach click handler to the parent of the scoped element
            $(this).parent().click(function(){

                // load the video
                loadVideo(this, videoid);
                $(this).unbind('click');
                $(this).find('.b-studio__image').hide();

            });
        }
    });
});

I've created a jsfiddle as well with all the code and some dummy data. Any suggestions would be really helpful.

\$\endgroup\$
  • \$\begingroup\$ Is this a .js file or a .php file, I ask because I see <?php the_field('two_vimeo_video_url'); ?> in there \$\endgroup\$ – konijn Dec 5 '14 at 18:32
  • \$\begingroup\$ It's a php file but I re-created the the data in the jsfiddle to match output. \$\endgroup\$ – RustyCollins Dec 9 '14 at 10:44
1
\$\begingroup\$

Just a small point, but in general, its best not to create anonymous functions. So for example:

$(function(){
    // Create array to store values
    var vimeoArray = [];
    var videoContainer = $('.video-container');

    // etc...

... should be contained inside a named function, and then called.
See Beware anonymous functions.

\$\endgroup\$
  • \$\begingroup\$ Ah good point, that's a good link as well, I'd never seen it before thanks Hywel! (i'd upvote if I had rep dude!) \$\endgroup\$ – RustyCollins Dec 5 '14 at 16:07
  • \$\begingroup\$ What the... The "Better" approach in that link is still using an anonymous function! I submitted a pull request to fix that ;) \$\endgroup\$ – konijn Dec 5 '14 at 18:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.