#571 ✓ staged
cadecairos

openmap plug-in rewrite

Reported by cadecairos | June 13th, 2011 @ 02:31 PM | in 0.8 (closed)

The openmap plugin has a lot of code in start which should be in setup, so I will move things that are better suited for set-up to that method.

Comments and changes to this ticket

  • Rick

    Rick June 18th, 2011 @ 11:34 AM

    • Milestone changed from 0.7 to 0.8
    • State changed from “new” to “assigned”
    • Milestone order changed from “38” to “0”

    Chris, you can delegate this to Dave or Cole if you'd like

  • cadecairos

    cadecairos July 7th, 2011 @ 11:38 AM

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

    I've moved a bunch of code to an _setup method so that the maps load (ideally) before start. In the event the map is not present when start or end is call I added a toggle method to check for it's existence.

    here's the branch: t571

  • Rick

    Rick July 8th, 2011 @ 03:10 PM

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

    Chris,

    On line 2, function ( should be function(

    Wherever there is } ), should be: }) (this is one of the exceptions)

    On line 64, "openmap" , should be "openmap",

        ... `){` should be `) {`
    

    On line 165, if( should be if (

    ... There are likely more, so I'll just trust you to sort through and find/fix :)

    Instead of checking for the _mapFired flag, perhaps test for OpenLayers...

    
    // insert openlayers api script once
    if ( !_mapFired ) {
      _mapFired = true;
      Popcorn.getScript( "http://openlayers.org/api/OpenLayers.js",
      function() {
        _mapLoaded = true;
      });
    }
    

    Suggesting:

    
    // insert openlayers api script once
    if ( !window.OpenLayers ) {
      Popcorn.getScript( "http://openlayers.org/api/OpenLayers.js", function() {});
    }
    

    And later, change the checks for _mapLoaded to look for window.OpenLayers

    I would cache the reference to: document.getElementById( options.target ) since you're using it more then once (if you cache it, make sure it's cached for each context that it's used in)

    I'll finish this review later today or this weekend, but you can nail these down whenever

  • cadecairos

    cadecairos July 11th, 2011 @ 11:05 AM

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

    Alright Rick, I've made a bunch of modifications on my t571 branch. Let me know if you spot some other things that should be fixed.

  • Rick

    Rick July 20th, 2011 @ 03:35 PM

    • Assigned user changed from “Rick” to “David Seifried”
    • State changed from “peer-review-requested” to “super-review-requested”
    • Milestone order changed from “1” to “0”

    Tested: openmap plugin test suite;

    Passing in:

    • FF 3.6 (stable), 5.x (stable), 6.x (Aurora), 7.x (Nightly)

    • Chrome 12 (stable), 13 (beta), 14 (Canary)

    Lint Passes

  • David Seifried

    David Seifried July 22nd, 2011 @ 02:40 PM

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

    Nice!

    Everything looks good to me, theres some styling issues in other parts of the code that you didnt touch, but everything that you touched looks good to me. I think ill file another ticket to go through all of the styling fixes ( tho there isnt too many ).

    Passes lint, works in ff 5 and chrome 12.
    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

Referenced by

Pages