#70 ✓ checked-in
Scott Downe

google maps street view

Reported by Scott Downe | June 23rd, 2010 @ 12:29 PM | in 0.4

Comments and changes to this ticket

  • annasob

    annasob June 23rd, 2010 @ 06:52 PM

    Ok so we need a consensus here. I like the street view thing but not sure how a user would be able to interact with the map and watch the video at the same time. Should we pause the video at the time when the user chooses to interact with the map or should this me a users choice with a default to yes. Im thinking maybe at the bottom of the web page we should have controls for things that can be turned on/off.

  • Scott Downe

    Scott Downe June 23rd, 2010 @ 07:31 PM

    We just let the user pause the video as they see fit, I would say.

    I havn't looked too close into it, but I think street view isn't even on by default, just an option a user can select on the video's controls, so if the user decides to use street view, they are doing something very specific, and should at their own peril. Finally, it's mostly an "oh neat" feature than something really practical; you're right, video is the focus.

  • Daniel H

    Daniel H June 23rd, 2010 @ 07:44 PM

    Don't focus to much on the ui, layout, player, etc IMO. I think we should just add an attribute to location for street view = true and if it's available do it.
    Street view is a feature or option for a map not something that has to be hardcoded or affect the player. Just like a zoom level is an option or map type so is street view.

    Sent from my iPhone

  • Scott Downe

    Scott Downe June 24th, 2010 @ 06:41 AM

    isn't street view something that happens when you zoom all the way in? Wouldn't this be the same? Right now if you zoom in all the way, you get "no image available" and not street view, I plan on getting us street view in that way, and not a static yes or no, one or the other toggle. Zooming in is my plan A. Plan B is to toggle with a true or false attribute.

  • Daniel H

    Daniel H June 24th, 2010 @ 08:01 AM

    If you zoom in to an area that does not have lower level detail or a street view you get no image available

    Not all roads have street view only a select bunch of cities in Canada, USA, Europe, etc. And in less Densely populated areas there isn't as fine detail maps

    You can't get street view by zooming in all the way through the API I believe that's only on google maps itself

    Sent from my iPhone

  • Daniel H

    Daniel H July 1st, 2010 @ 06:55 PM

    • Milestone cleared.
    • Milestone order changed from “0” to “0”

    moving to 0.2

  • annasob

    annasob November 3rd, 2010 @ 11:24 AM

    • Milestone cleared.

    @@@ @@@ [bulk edit]

  • annasob

    annasob November 3rd, 2010 @ 11:25 AM

    • Milestone set to 0.2 release
    • Milestone order changed from “8” to “0”
  • Scott Downe

    Scott Downe November 19th, 2010 @ 11:47 AM

    • Milestone changed from 0.2 release to 0.3 release date
    • Milestone order changed from “5” to “0”
  • Scott Downe

    Scott Downe January 18th, 2011 @ 10:38 AM

    • State changed from “assigned” to “open”
    • Assigned user cleared.
  • Scott Downe

    Scott Downe January 18th, 2011 @ 10:38 AM

    • Tag set to starter
  • dseif

    dseif January 20th, 2011 @ 11:39 AM

    • Assigned user set to “dseif”
  • dseif

    dseif January 20th, 2011 @ 11:39 AM

    • no changes were found...
  • dseif

    dseif January 20th, 2011 @ 11:40 AM

    • Assigned user changed from “dseif” to “David Seifried”
  • annasob

    annasob February 8th, 2011 @ 05:32 PM

    • Milestone changed from 0.3 release date to 0.4
  • dseif

    dseif February 10th, 2011 @ 09:20 PM

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

    Dont know if this is how im supposed to post stuff for peer review but here goes:

    commit: https://github.com/dseif/popcorn-js/commit/e15f298aeb11931cb65a4e8d...
    tree: https://github.com/dseif/popcorn-js/tree/bug70/plugins/googlemap

    I changed the html file to include a demo of streetview as well, hope thats ok.

  • annasob

    annasob February 11th, 2011 @ 04:23 PM

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

    Hey dseif this code looks good and works. However before I can review thoroughly your commit will need to be redone :(. This is mainly due to the fact that when you look at the differences in your commit you will see things like:
    - // If there is no lat/lng, and there is location, geocode the location 84
    + // If there is no lat/long, and there is location, geocode the location

    This is undoing the changes that were made in a different ticket. Currently we are using "lng"

    It seems that perhaps your branch wasn't up-to-date before you started with this. If you need help with this you can come by or I can email you the steps to do this.

    Also here is a couple nit picks related to your code:
    - you added a options.heading and options.pitch parameter but you didn't add them to the comments at the top of the .js file - it appears that you are not using the rule tab = 2 spaces

    Please fix :)

  • dseif

    dseif February 11th, 2011 @ 04:26 PM

    alright will do anna, thanks for the input :)

  • dseif

    dseif February 11th, 2011 @ 06:53 PM

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

    Alright I think I did everything you mentioned, so heres the new commit:
    https://github.com/dseif/popcorn-js/commit/f575b14a07c2f10ea93157b1...

  • Rick

    Rick February 12th, 2011 @ 03:26 PM

    I think this change was made directly on your 0.4 branch instead of the bug70 branch

  • Rick

    Rick February 12th, 2011 @ 03:43 PM

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

    I can confirm my previous question - I had to pull the 0.4 branch to get the lastest changes, please merge that into your bug70 branch. From your project directory, run this:

    
    git checkout bug70 && git checkout -b scratch70 && git merge 0.4
    

    If that works and does not result in any conflicts then it is safe to merge back into bug70 and push the branch up to github.

    
    git checkout bug70 && git merge scratch70 && git push origin -u bug70
    

    As for the code...

    In popcorn.googlemap.js, there are some indentation issues...

    
        location : {elem:'input', type:'text', label:'Location'},
    heading  : {elem:'input', type:'text', label:'Heading'},
    pitch    : {elem:'input', type:'text', label:'Pitch'}
      }
    },
    

    Should be...

    
        location : {elem:'input', type:'text', label:'Location'},
        heading  : {elem:'input', type:'text', label:'Heading'},
        pitch    : {elem:'input', type:'text', label:'Pitch'}
      }
    },
    

    As I continue, it appears there are a lot of inconsistent indentations and broken whitespace rules. Please read the style guide: https://webmademovies.lighthouseapp.com/projects/63272/styleguide

    Flaring indentation issues on L143-152

    Please update your code accordingly. It's not explicitly listed (though probably should be) but please keep your quotes consistent. I realize they may have been mixed prior to your changes, but you can be a hero and make sure they are all consistently double quotes, only using single quotes when double quotes are present in the string.

  • dseif

    dseif February 13th, 2011 @ 03:02 PM

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

    Hey Rick,

    Thanks for the tips and linking the style sheet, definitely made stuff clearer.

    Commit: https://github.com/dseif/popcorn-js/commit/c0ecc88cc16ef411008c8970...
    Tree: https://github.com/dseif/popcorn-js/tree/c0ecc88cc16ef411008c8970ae...

  • Rick

    Rick February 13th, 2011 @ 09:13 PM

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

    There are now merge conflicts in the source code:

    https://github.com/dseif/popcorn-js/commit/c0ecc88cc16ef411008c8970...

    There are problems here:

    https://github.com/dseif/popcorn-js/blob/c0ecc88cc16ef411008c8970ae...

    1. The indentation is off

    2. The properties are set with the assumption that all of those option properties have values - shouldn't we check those?

    3. Do we need to create a variable for something that is only used once?

    4. options._map.setStreetView( streetView ); should not be called outside of the if block - streetView doesn't exist unless we enter that block. I checked to see what was happening to the undefined streetView in the demo html and apparently google maps API just swallows the bogus call. I'm not ok with that, so let's be sure that never gets called unless streetView is actually defined.

    5. While I'm on the subject of vars that are only used once, let's save some trim the fat so to speak :D

    This...

    
    if( options.type === "STREETVIEW" ){
      var streetViewOptions = {
      position: options._location,
      pov: {
        heading: options.heading,
        pitch: options.pitch,
        zoom: options.zoom
        }
      };
      var streetView = new  google.maps.StreetViewPanorama( options._newdiv, streetViewOptions );
    }
    options._map.setStreetView( streetView );
    

    Should look like:

    
    // If map request is "STREETVIEW", enter into special case handling
    if ( options.type === "STREETVIEW" ) {
    
      // Switch this map into streeview mode
      options._map.setStreetView( 
        // Pass a new StreetViewPanorama instance into our map
        new google.maps.StreetViewPanorama( options._newdiv, {
          position: options._location,
          pov: {
            // I would suggest providing a lazy check and a default value here
            // ex: heading: options.heading && options.heading || __some default literal value__, 
            heading: options.heading,
            pitch: options.pitch,
            zoom: options.zoom
          }    
        })
      );
    }
    

    Also - your patience and commitment to this feature are voracious - that's really awesome, keep it up and don't get discouraged. It takes time to figure out the flow :)

  • dseif

    dseif February 13th, 2011 @ 09:37 PM

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

    Hey Rick,

    Thanks again for all the help, I'm slowly but surely getting the hang of everything ahah.

    Are you in the popcorn IRC channel at all? If so whats your nick?

    Thanks again!

  • Rick

    Rick February 13th, 2011 @ 10:25 PM

    I'm in the popcorn channel 24/7, nick is "rwaldron"

  • dseif

    dseif February 13th, 2011 @ 11:50 PM

    Heres the commit, hope its better!
    Added some checks to make sure heading and pitch were both numbers and used the code you supplied to remove any variables that were only used once.
    Commit: https://github.com/dseif/popcorn-js/commit/e5ba8c2317b2cbcf14d9ba31...

    Thanks again for the help!

  • dseif
  • Rick

    Rick February 14th, 2011 @ 04:10 PM

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

    Perfect.

  • Scott Downe

    Scott Downe February 14th, 2011 @ 04:29 PM

    • State changed from “super-review-requested” to “under-review”
    • Assigned user changed from “David Seifried” to “Scott Downe”
  • Scott Downe

    Scott Downe February 14th, 2011 @ 04:36 PM

    • Assigned user changed from “Scott Downe” to “annasob”
  • annasob

    annasob February 14th, 2011 @ 04:45 PM

    • State changed from “under-review” to “staged”
    • Assigned user changed from “annasob” to “David Seifried”

    This looks good SR+

    Staged in annasob/popcorn-js commit

  • annasob

    annasob March 21st, 2011 @ 02:47 PM

    • State changed from “staged” to “checked-in”
  • annasob

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

Pages