#734 ✓ staged
Rick

DRY-out Facebook plugin code

Reported by Rick | September 23rd, 2011 @ 09:55 AM | in 0.9 (closed)

There are a number of opportunities for DRY-out in this plugin, most obviously the switch block needs to go and the code within it needs to be refactored.

Comments and changes to this ticket

  • mjschranz

    mjschranz September 25th, 2011 @ 09:33 PM

    Discussed with dseif over IRC about what exactly was wanted for the ticket as I don't really know what is meant by DRY-out at this stage.

    His comments were:

    • Repeated Code where we could store a reference to it instead of repeating the same code over and over again With that I wasn't sure 100% what he ment. At first I thought he ment simply having variables storing the values from the passed in options so I cleaned that up in this commit here. Really minor obviously but let me know if there was something else intended.

    • He also mentioned the toggle function. I'm still figuring stuff out so I'm assuming he means the return block here. I wouldn't know how to change that at this point but it's something I'll be looking into.

    Anyway, any clarification for the ticket would be great.

    Thanks

  • David Seifried

    David Seifried September 25th, 2011 @ 09:38 PM

    Essentially there are a few spots that could use cleaning up here.

    The toggle function at the top probably doesnt need to be there. We should just be able to set the style.display to either inline or none depending on either start or end. There may be some reason for this that I am not seeing, but on first glance, it doesnt seem necissary.

    From 147 - 189, we are repeating a ton of code. We can definitely store a reference to the container object and at first look there seems to a lot of attributes that are repeated in other blocks here. This whole block can probably be written in a better way.

    From 123 - 133, this can be written a bit better as well I think. In fact I don't know if we want it to default to the like plugin at all if the user passes in a bogus type. This is probably a better discussion for channel.

    There is probably a few more instances where things could be written better, just continue to look over the code for instances where stuff may not be necissary and where code is repeated. This will probably require you to step through the code and really undestand how it is working. The good news is afterwards you will be an expert on the facebook plugin, as none of us are at the moment.

  • Rick

    Rick September 25th, 2011 @ 10:25 PM

    That's an excellent list. Keep in mind the test runner is there to help you - all the current tests should pass when you're done, but run it while you develop too!

  • mjschranz

    mjschranz September 26th, 2011 @ 10:56 AM

    Toggle function appeared to be doing nothing. I removed it and just changed the start and end functions to set everything manually for now until I know how I would make it so they could be set at run time based on start/end values (that is if I'm understanding David correctly).

    Code no longer sets the plugin type to "like" whenever an invalid one is passed, now throwing an error for it. The old tests that were invalid types and defaulted to "like" were removed and one has been added for invalid plugin types.

    Still don't know what to do with that giant return block in setOptions but I guess I'll leave that for now.

    Commit changes found here.

  • mjschranz

    mjschranz September 26th, 2011 @ 05:28 PM

    • State changed from “assigned” to “peer-review-requested”
    • Tag set to facebook plugin
    • Assigned user changed from “mjschranz” to “David Seifried”

    Finished reworking the return block of code and all seems to be good. Passed all tests on chrome and firefox on my machine so for now I'll submit it for review!

    Commit found here.

  • David Seifried

    David Seifried September 26th, 2011 @ 05:48 PM

    Nice job man! I will review this first thing in the morning.

  • David Seifried

    David Seifried September 27th, 2011 @ 11:15 AM

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

    So it looks like since stuff got staged since you began working on this, that were getting a merge conflict now. This is a good opportunity for you to learn how to fix a merge conflict, so im going to throw back to you to get this sorted out :)

    Come into channel if you need some help on fixing it.

  • mjschranz

    mjschranz September 27th, 2011 @ 12:25 PM

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

    Merge conflicts should be fixed as far as I can tell.

    New branch is located here, t734b.

  • mjschranz

    mjschranz September 27th, 2011 @ 04:21 PM

    NOW it seems merge conflicts are solved.

    There's also other things that creeped up about missing features so that will have to wait for a new ticket and be for 1.0 release.

    Anyway, here's the final branch.

    https://github.com/mjschranz/popcorn-js/tree/t734d

  • David Seifried

    David Seifried September 27th, 2011 @ 05:07 PM

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

    Nice job man!

    Passes lint, unit tests pass in FF 6 and Chrome 14. Style looks perfect.

    PR+

  • cadecairos

    cadecairos September 28th, 2011 @ 10:18 AM

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

    I'm questioning why setOptions is a function. It doesn't look like the logic in there depends on anything happening between it's definition and it's call. We can safely un-wrap it in that function.

    also, we clear each instance on end ( via options._container.innerHTML = ""; ), effectively destroying it. This should be happening in _teardown. All instances should be wrapped in a new div before being appended to the target div, and each supplied with a unique GUID that can be used to remove it when it's teardown is called. The way it is now will remove every facebook widget in a single target div when the first instance hits end.

  • mjschranz

    mjschranz September 28th, 2011 @ 12:30 PM

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

    As far as I know I have your requests fixed Chris. Although, based on previous tries with this might not actually be true!

    No whitespace either when checking with \s+$.

    Commit found here.

  • David Seifried

    David Seifried September 28th, 2011 @ 12:47 PM

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

    Since multiple facebook plugins can be inside one target div, we need to create an inner container div to keep track of these as chris suggessted.

    Gonna throw back to you to add this in.

    Also since a teardown has been added, we are going to need some tests to make sure this functionality is working as expected.

  • Rick

    Rick September 28th, 2011 @ 05:16 PM

    @mjschranz

    You took off from IRC, but the changes remaining are minimal...

    In your tests, change:

    
    ok( !facepilediv, "removed facebook social plugin was properly destroyed" );
    

    to:

    
    ok( !document.getElementById( "facepilediv" ).innerHTML, "removed facebook social plugin was properly destroyed" );
    

    Then in popcorn.facebook.js:

    
    target && target.removeChild( options._facebookdiv );
    

    to:

    
    target && target.removeChild( options._container );
    
  • mjschranz

    mjschranz September 28th, 2011 @ 07:17 PM

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

    _teardown added and test for it added as well.

    All tests pass Chrome 14 FF 6.

    No whitespace issues or other style stuff from what I can tell.

    I hope it's done, although this has seriously opened my eyes in many respects.

    Edit: I noticed later that the tests weren't bringing up any of the other facebook plugin types so I added those and noticed a bunch of other tests I could be doing while looking over the other plugins so I added all that as well.

    Latest commit here.

  • cadecairos

    cadecairos September 29th, 2011 @ 11:01 AM

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

    This looks great! good stuff dude.

    Tested the Facebook plug-in unit tests on:

    Firefox 7, Aurora, Chrome 14

    All passed.

    passes lint.

    Code looks good.

  • cadecairos
  • Rick
  • Rick
  • Rick

    Rick October 31st, 2011 @ 05:33 PM

    (from [81e7569896c0efad516cfc870775e30291aeb81d]) [#734] Made reference variables for all plugin options rather than having option.someValueHere all throughout the code. https://github.com/rwldrn/popcorn-js/commit/81e7569896c0efad516cfc8...

  • Rick
  • Rick

    Rick October 31st, 2011 @ 05:33 PM

    (from [bc68cb529a6f2a752a61e03929841c060842b944]) [#734]Code Refactoring Complete. Return block gone replaced with a more stream lined loop of code. https://github.com/rwldrn/popcorn-js/commit/bc68cb529a6f2a752a61e03...

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

Pages