#516 ✓ staged
Mohammed Buttu

Flickr plugin: getting images by tags not displaying

Reported by Mohammed Buttu | May 11th, 2011 @ 03:15 PM | in 0.7

In popcorn-js/plugins/flickr/popcorn.flickr.html, The following code is used to display images tagged 'georgia', but nothing is displayed:

      .flickr({
        start: 20, // seconds
        end: 30, // seconds
        tags: 'georgia',
        numberofimages: '8',
        target: 'flickrdiv'
      } )

Comments and changes to this ticket

  • Rick

    Rick May 11th, 2011 @ 03:23 PM

    • State changed from “new” to “assigned”
    • Assigned user set to “Mohammed Buttu”

    Mohammed, I'm assigning this to you to investigate. Also, I'd suggest changing the string 8 to a number 8 and all references in the plugin and demo. :)

  • Mohammed Buttu

    Mohammed Buttu June 16th, 2011 @ 01:48 PM

    • State changed from “assigned” to “peer-review-requested”
    • Assigned user changed from “Mohammed Buttu” to “David Seifried”
  • Rick

    Rick June 16th, 2011 @ 04:05 PM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Mohammed Buttu”
  • Mohammed Buttu

    Mohammed Buttu June 16th, 2011 @ 05:19 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Mohammed Buttu” to “David Seifried”

    Hi Rick:

    No, there is no dependency between tickets 515 and 516.

  • Rick

    Rick June 16th, 2011 @ 05:59 PM

    Gotcha, I'm more concerned about merge conflict.

  • David Seifried

    David Seifried June 17th, 2011 @ 11:20 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “David Seifried” to “Mohammed Buttu”

    Ok so a few things, in the popcorn.flickr.html and popcorn.flickr.unit.js we can probably remove all of the // Seconds comments and just add it to the definitions of Start and End at the top of the popcorn.flickr.js file. It seems a bit redundant writing the same comment 10x.

    In the popcorn.flickr.js file:

    On line 59, mind adding a space between "flickr" and the + sign

        containerDiv.id            = "flickr"+ i;
    

    On line 78, there needs to be whitespace padding around data

            Popcorn.xhr.getJSONP( _uri, function(data) {
    

    Line 85 needs a single whitespace added after the first round bracket

            setTimeout(function () {
    

    On 148 containerDiv is indented 4 spaces instead of 2

          end: function( event, options ){      
              containerDiv.style.display = "none";       
          },
    

    Other than those small styling fixes and removing those comments everything looks good!

  • Mohammed Buttu

    Mohammed Buttu June 17th, 2011 @ 11:56 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “Mohammed Buttu” to “David Seifried”

    Ready for review, branch t516 (https://github.com/mbuttu/popcorn-js/tree/t516/plugins/flickr), but a few comments:

    1) In the popcorn.flickr.js file, lines 49 - 53, I removed the white spaces between the variable name and the equal sign, and
    2) Regarding your comment for adding a single white space at line 85 after the first round bracket, it is an exception in the style guide:

    
      Paren whitespace exceptions
      --------------------------------------------------------------------
    
      //  Functions with callbacks 
      foo(function() { 
      
      });
      
      //  Functions accepting arrays
      foo([   ]);
      
      //  Functions accepting object
      foo({   });  
    
    3) I also added a space after the last argument on line 87 in popcorn.flickr.js
    
  • David Seifried

    David Seifried June 17th, 2011 @ 03:06 PM

    • State changed from “peer-review-requested” to “super-review-requested”
    • Assigned user changed from “David Seifried” to “cadecairos”

    Alright this looks awesome!
    Tested on FF 4 and Chrome 11, also linted.

    PR+.

  • cadecairos

    cadecairos June 20th, 2011 @ 10:21 AM

    • State changed from “super-review-requested” to “review-needs-work”
    • Assigned user changed from “cadecairos” to “Mohammed Buttu”

    Everything looks great. can you make one quick change:

    on line 100,

    Popcorn.xhr.getJSONP( _uri, function( data ) {
    

    Popcorn.xhr.getJSONP(... can be shortened to Popcorn.getJSONP(...

    Then its good to go.

  • Mohammed Buttu

    Mohammed Buttu June 20th, 2011 @ 10:37 AM

    • State changed from “review-needs-work” to “super-review-requested”
    • Assigned user changed from “Mohammed Buttu” to “cadecairos”
  • cadecairos

    cadecairos June 20th, 2011 @ 10:41 AM

    • State changed from “super-review-requested” to “review-looks-good”
    • Assigned user changed from “cadecairos” to “Jon Buckley”

    Perfect!

    Tested Flickr Unit tests on:

    Firefox 4, Chrome 12

    All tests pass.

    Checked flicker againt JSLint, passed.

    SR+

  • Jon Buckley

    Jon Buckley June 20th, 2011 @ 04:19 PM

    • State changed from “review-looks-good” to “review-needs-work”
    • Assigned user changed from “Jon Buckley” to “Mohammed Buttu”

    I'm getting a merge conflict that I don't feel comfortable resolving myself:

    <<<<<<< HEAD
            _border  = options.border || "0px";
    
        // create a new div this way anything in the target div is left intact
        // this is later populated with Flickr images
        containerDiv               = document.createElement( "div" );
        containerDiv.id            = "flickr" + idx;
        containerDiv.style.width   = "100%";
        containerDiv.style.height  = "100%";
    =======
            _border = options.border || "0px",
            i;
    
        // create a new div this way anything in the target div is left intact
        // this is later populated with Flickr images
        containerDiv = document.createElement( "div" );
        containerDiv.id = "flickr" + i;
        containerDiv.style.width = "100%";
        containerDiv.style.height = "100%";
    >>>>>>> mbuttu/t516
    

    We can do this tomorrow.

  • Jon Buckley

    Jon Buckley June 21st, 2011 @ 10:38 AM

    • State changed from “review-needs-work” to “staged”

    https://github.com/jbuck/popcorn-js/commit/b6c8afa84fb3cca237cc8391...

    Fixed the merge conflict with Mohammed's help, I was reading it wrong, heh.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Popcorn.js is an HTML5 video framework that lets you bring elements of the web into your videos.

Popcorn.js is a project of Web Made Movies, Mozilla's Open Video Lab.

Shared Ticket Bins

Referenced by

Pages