#331 ✓ staged
David Humphrey

Create a Facebook plugin

Reported by David Humphrey | February 12th, 2011 @ 05:19 PM | in 0.6

This may need to get split into multiple tickets, as I'm not sure how extensive their API is (I don't use Facebook), but we should look at how we can query and display certain kinds of data from Facebook in a plugin.

Comments and changes to this ticket

  • danman
  • danman

    danman March 16th, 2011 @ 02:07 PM

    • Assigned user set to “danman”

    https://github.com/DanVentura/popcorn-js/tree/3ad710c74d6512d4d28a3...

    Four possible facebook plugins as of now, found from here: Facebook Plugins. Didn't add the other ones because they require some kind of site registration. That functionality can be added at some point, but this is a start.

  • danman

    danman March 16th, 2011 @ 02:08 PM

    • State changed from “bugs” to “peer-review-requested”
  • Rick

    Rick March 31st, 2011 @ 09:19 PM

    • Milestone set to 0.6
    • Milestone order changed from “29” to “0”
  • Steven W

    Steven W April 13th, 2011 @ 04:44 PM

    Nice plugin, seems to work great! I had an issue with type="FACEPILE" displaying on Firefox 4, not sure if it's on FB's side or not.

    A few thoughts:
    - If "type" isn't specified, the plugin crashes. Could we default it to a valid type, and document it at the top of the plugin?
    - Could you write a few unit tests for this plugin and add it to plugins/index.html? This will help protect against the plugin breaking in the future.


    The GoogleMap plugin seems similar to this in that multiple "type"s are possible, it could be a good place to base tackling these from.

  • Steven W

    Steven W April 13th, 2011 @ 04:44 PM

    • State changed from “peer-review-requested” to “review-needs-work”
  • danman

    danman April 13th, 2011 @ 06:15 PM

    Will make those changes asap.
    Facepile only renders if you're logged into Facebook. I think it's a design fault on fb's account because other plugins will show a sign in button. If I can find a way to check if the current user is logged in or not I could put an error message or sign in button.

  • Rick

    Rick April 13th, 2011 @ 06:18 PM

    Additionally, you'll need to create the required unit tests for this plugin (see the others for examples)

  • Rick

    Rick April 13th, 2011 @ 06:26 PM

    Please use the build-in Popcorn.getScript() api for loading remote script resources...

    
    (function() {
      var e = document.createElement( 'script' );
      e.setAttribute( 'src', 'http://connect.facebook.net/en_US/all.js' );
      e.setAttribute( 'async', 'true' );
      document.getElementById( 'fb-root' ).appendChild( e );
    }());
    
    
    Popcorn.getScript("http://connect.facebook.net/en_US/all.js" /*, optional callback */ );
    

    Tidy up the single/double quotes, preferably change them all to double quotes

    I like your approach with "setOptions" (for code organization).

  • Steven W

    Steven W April 13th, 2011 @ 06:50 PM

    I can't say what the issue is, but I was logged in at the time. Definitely an inconsistency in their API though, compared to other capabilities (like the Like button). When I tried running it the loading bar for the Facepile container ran, followed by an empty white rectangle, which was later hidden entirely (the container below it shifted up to fill its space).

  • danman

    danman April 13th, 2011 @ 11:16 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    https://github.com/DanVentura/popcorn-js/commit/a4947d4a34c034c922e...

    Changes:
    - single quotes changed to double (assuming i didn't miss any)

    • facebook.js uses getScript

    • null types default to like button. invalid types return nothing

    • added unit tests and added tests to index.html

    Steve, I haven't been able to produce the same facepile behavior as you mentioned. Does it display properly on this page?: http://developers.facebook.com/docs/reference/plugins/facepile/

  • Rick

    Rick April 20th, 2011 @ 10:30 AM

    • State changed from “peer-review-requested” to “review-needs-work”

    Auto-merging plugins/index.html
    CONFLICT (content): Merge conflict in plugins/index.html
    Automatic merge failed; fix conflicts and then commit the result.

  • danman

    danman April 20th, 2011 @ 01:09 PM

    • State changed from “review-needs-work” to “peer-review-requested”
  • Rick

    Rick April 21st, 2011 @ 09:45 AM

    • State changed from “peer-review-requested” to “review-needs-work”

    Thanks Dan, can you have a look at the styles being applied?

    http://gyazo.com/bb033cfdd2a0caad198ccbc89cff0979.png

  • danman

    danman April 21st, 2011 @ 02:22 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    https://github.com/DanVentura/popcorn-js/commit/d8bbdf45ebf6aa8dd68...

    My bad. Removed absolute top style to keep from overlapping.

  • annasob

    annasob May 5th, 2011 @ 12:34 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    Hey Dan this looks awesome!
    Couple of changes need to happen b4 this lands though:

    • in the facebook.js file u have an documented example using: show_faces: "true",header: "false" however in the written part under options you do not mention these (manifest as well). I also see width | 450 this default needs to be documented not sure how this will look with the different types/defaults

    • line 86 should do tolowercase() or touppercase() just in case the user puts Like-Box

    if( options.type === "LIKE-BOX" )
    
    • where is the start and end function? the plugin needs to have this.

    Can you also explain what the Recent Activity is suppose to be doing and why it is linked to popcornjs.org. I get a blank box in that even when signed in to FB. How is it going to get activity from popcornjs.org?

    I know this is a lot of "please fix" but it is an exciting plugin that will get a lot of attention.

  • danman

    danman May 10th, 2011 @ 10:40 PM

    • State changed from “review-needs-work” to “peer-review-requested”

    https://github.com/DanVentura/popcorn-js/commit/54eaf54ffcb4aa8f2de...

    • Added options to manifest and gave a short description of each.
    • Removed setting default values where unnecessary. Passing "undefined" to facepile's max_rows parameter and like-box's height parameter cause problems, so I kept the defaults for those.
    • Added a line to make options.type lower case earlier on to avoid multiple toLowerCase() calls.
    • Added start and end functions.
    • Fixed a bug regarding the activity feed. Turns out it uses "site" rather than "href", oddly.
    • Revised unit facebook tests

    The activity feed lists pages related to the specified url (popcornjs.org) that have been shared on facebook (now that it works, haha). For example, one of the results I get is popcornjs.org/demos.

  • annasob

    annasob May 15th, 2011 @ 01:04 PM

    • State changed from “peer-review-requested” to “review-needs-work”

    Hey Danman the changes look good, thanks for the activity feed explanation. I am still concerned about a couple of things:
    - the user will never be able to get at appId unless they hack at the code. Could we add this to the options? - also overriding options.type, even though it saves you having to make a new variable, is not ideal. This is due to the fact that people using the plugin will expect the types to be what they originally passed in to the plugin. I am referring to :

    options.type = options.type.toLowerCase();
    
    • I am not sure if I like the splitting up of comments. Where the example is in-between the comment block. However line 29 doesn't have a "*" beside it which breaks the pattern

    I didn't realize that popcornjs.org was all over facebook link shared by 116 people.

  • annasob

    annasob May 15th, 2011 @ 01:11 PM

    Rick can you also take a look at this and provide feedback so that dan can make all of the changes at once?

  • Rick

    Rick May 16th, 2011 @ 03:02 PM

    • State changed from “review-needs-work” to “under-review”
    • Assigned user changed from “danman” to “Rick”
  • Rick

    Rick May 16th, 2011 @ 04:25 PM

    • State changed from “under-review” to “review-needs-work”
    • Assigned user changed from “Rick” to “danman”

    At some point during the test, this exception is being thrown from the start method:

    Uncaught TypeError: Cannot read property 'style' of undefined popcorn.facebook.js:179

    and this from the end method:
    Uncaught TypeError: Cannot read property 'style' of undefined popcorn.facebook.js:188

  • cadecairos

    cadecairos May 17th, 2011 @ 11:17 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user cleared.

    I added checks to avoid the undefined problem, and added unit tests.

    the commit is here: https://github.com/cadecairos/popcorn-js/commit/6c1941dbb6f15fba25f...

  • Rick

    Rick May 17th, 2011 @ 11:33 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user set to “cadecairos”

    @cadecairos, you could DRY that out with a single function (defined inside the closure, just before the Popcorn.plugin factory call)...

    
    function toggle( display )
    
      if ( options._container ) {
        options._container.style.display = display;
    
        return;
      }
    
      setTimeout(function() {
        toggle( display );
      }, 10 ); 
    }
    

    :)

  • cadecairos

    cadecairos May 17th, 2011 @ 11:49 AM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “cadecairos” to “Rick”

    I have made the recommended changes to start and end. see the commit

    I also added newlines to all the popcorn.facebook.* files in this commit (these changes are included in the above commit)

  • Rick

    Rick May 17th, 2011 @ 11:59 AM

    • State changed from “peer-review-requested” to “review-needs-work”
    • Assigned user changed from “Rick” to “cadecairos”

    You need to split out that "popcorn.videosequencer" stuff.

    Also - I'd prefer that "popcorn.videosequencer" (btw, the correct name is "Popcorn.sequence" ) was a submodule that the is updated via the Makefile

    Aside from that, the DRY out is really nice - i like that you abstracted the element to a param as well.

  • cadecairos

    cadecairos May 17th, 2011 @ 12:01 PM

    I didn't realize that was in there, that's some old old work. I'll get rid of it.

  • cadecairos

    cadecairos May 17th, 2011 @ 12:07 PM

    • State changed from “review-needs-work” to “peer-review-requested”
    • Assigned user changed from “cadecairos” to “Rick”

    The "videosequencer" folder + files have been removed.

    commit

  • Rick

    Rick May 17th, 2011 @ 12:12 PM

    • State changed from “peer-review-requested” to “under-review”

    This looks good, I'm still curious about the APP ID stuff though, is there a consensus on this?

  • Rick

    Rick May 17th, 2011 @ 12:15 PM

    Also - nice job... I ran this through JSHint (yes, hint) and it passed 100% - if you try it, be sure to include: /global Popcorn: true, FB: true /

  • danman

    danman May 17th, 2011 @ 02:47 PM

    https://github.com/DanVentura/popcorn-js/blob/t331/plugins/facebook...

    • Merged changes from cadecairos/facebook

    • Added app_id capability

    • Added more plugins: live-stream (app_id required), send button

    • Added comments plugin but since has been removed due to loading issues. The plugin never finishes loading. I think it may be a problem with the plugin itself.

    • Updated documentation in comments for extra options added

  • Rick
  • Rick

    Rick May 18th, 2011 @ 02:34 PM

    • State changed from “under-review” to “peer-review-requested”
    • Assigned user changed from “Rick” to “annasob”
  • Rick
  • annasob

    annasob May 18th, 2011 @ 02:51 PM

    • State changed from “peer-review-requested” to “review-looks-good”

    Ok rick said its PR+ with his patch. I re-reviewed and it looks awesome! I posted using the plugin and it updated my facebook! NICELY DONE Danman!!!!

  • annasob
  • annasob

    annasob May 18th, 2011 @ 02:55 PM

    • State changed from “review-looks-good” to “staged”

    Staged in annasob/popcorn-js commit

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

Tags

Referenced by

Pages