#657 ✓ staged
Brett Gaylor

Change popcorn.Footnote to popcorn.Text

Reported by Brett Gaylor | July 19th, 2011 @ 07:47 PM | in 0.8 (closed)

Its our most common plugin, but calling it footnote is legacy from our original demo. Lets just call it "text"

Comments and changes to this ticket

  • Rick

    Rick July 19th, 2011 @ 08:11 PM

    Just to be thorough - this isn't as straight foward as simply renaming the plugin and its files - we have a lot of code out in the wild that is using footnote.

    Any ideas?

  • Scott Downe

    Scott Downe July 20th, 2011 @ 10:15 AM

    We cannot remove footnote, nor do I want to. We can however, create a text plugin.

    This is much like why we made compose and effect. They are the same thing, but their two different namespaces make it easier for people to find what they need depending on their usecase.

    The only problem with making a new plugin that is the same as another, is maintenance. If down the road footnote needs a change, their is a chance that same change will not make it into text.

    I think we need to brainstorm ideas fo a way to wrap a plugin, and give it a new name, while keeping the original, and allowing them to share the same code.

  • Rick

    Rick July 20th, 2011 @ 11:39 AM

    @Scott,

    I think you could actually just do something like...

    
    Popcorn.forEach( [ "footnote", "text" ], function( plugin ) {
    
      Popcorn.plugin( plugin, /* .... put the footnote definition here ... */ );
    
    });
    

    And ta-da! 2 for the price of 1 :)

  • Rick

    Rick July 20th, 2011 @ 02:28 PM

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

    Rick July 20th, 2011 @ 03:33 PM

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

    Scott Downe July 20th, 2011 @ 03:37 PM

    Rick, this is an almost perfect solution.

    The only loose end I see, and I am not sure we care, is that the script is still going to be named footnote.js.

    But that's pretty trivial considering most people that will get hung up on names, are people that are using popcorn-complete.

    So this just may be enough. Thoughts?

  • Rick

    Rick July 20th, 2011 @ 06:24 PM

    I was betting on the popcorn-complete.js horse.

  • David Seifried

    David Seifried July 21st, 2011 @ 06:29 PM

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

    Really cool fix, passes lint, tested in firefox 5, chrome 12. The only thing that could use some changing I think, is in the demo we should show the functionality using both .footnote({}) and .text({}). Other then that, everything looks good.

    Gonna throw to Chris for SR pending that change gets done.

  • Rick

    Rick July 21st, 2011 @ 07:46 PM

    Thanks :) Let's file a ticket to have someone update the demo separately

  • cadecairos

    cadecairos July 22nd, 2011 @ 11:36 AM

    should we update the plugins/index.html file with the changes we have made?

  • Brett Gaylor

    Brett Gaylor July 22nd, 2011 @ 12:43 PM

    Hi, all! My original concern was certainly that the naming "footnote" is odd, thinking towards 1.0 it seems like the plugin should reflect that. It seems like the proposed solution works from a technical perspective.

    I do wonder whether this will work with the "build tool" we've discussed - since it relies on popcorn-complete.js . I'll file a meta-ticket or bring this up on our next call, as we're looking at different models of build tools, some like http://www.modernizr.com/

  • Rick

    Rick July 22nd, 2011 @ 01:19 PM

    @Brett

    It's not that it "relies" on Popcorn-complete.js, it's that there is no dedicated file called "popcorn.text.js", so if someone were looking for that file to use just the popcorn.text() plugin, they wouldn't find it...

    The person would have to include popcorn.footnote.js to get the popcorn.text() plugin. This can easily be handled with plugin docs, wherein we create a unique page for popcorn.text() that clearly states something like:

    If you'd like to use the Popcorn Text plugin, but are not using popcorn-complete.js in your project, simply include popcorn.footnote.js in your HTML page. The Popcorn "Footnote" and "Text" plugins are identical in every way with the exception of their name, therefore we ship them together for efficiency.

  • cadecairos

    cadecairos July 22nd, 2011 @ 03:05 PM

    Other than plugin/index.html and the demo page needing updates the code looks good.

    All footnote/text unit test passing in Firefox 5 and Chrome 13.

    Passes Lint check

    Should we deal with updating those html files in another ticket?

  • cadecairos

    cadecairos July 27th, 2011 @ 11:11 AM

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

    We can deal with updating the html files elsewhere, I'm not going to let this sit here for any longer.

    Re-tested footnote/text unit tests on:

    Firefox 5, 8 - PASS
    Chrome 13 - PASS

    Passes lint

    SR+

  • Jon Buckley

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