open-austin / voteatx-app

Voting place finder application for Travis County, TX
http://voteatx.us/
Other
4 stars 2 forks source link

support query argument for current date/time #2

Closed avickers closed 10 years ago

avickers commented 10 years ago
courtney-rosenthal commented 10 years ago

Pull request #4 addresses this.

avickers commented 10 years ago

I already added a method of splitting queries for geolocation:

var URL = window.location.toString();

var queries = URL.split("?");
        if(queries[1] != undefined){
            var params = queries[1].split("&");
            var query = params[0].split("=");
            if(query[0] === "g" || "geo"){
                var GEOLOCATION = query[1];
            } 
        }

Your code is designed to handle multiple queries, where as I avoided handling that to keep the code base smaller, though your approach is very compact! The issue with your implementation here is that window.onpopstate is implemented differently across browsers. In Chrome and Safari, it will trigger on page load. In Firefox and IE, it will not. Thus, it is not a good production solution because geolocation will not work in all browsers if we go with this approach, and using both violates DRY.

We should only implement a window.onpopstate listener if we actually need it in production. If it is just going to be for testing, it would make more sense to use a test branch.

If we can envision a reason for multiple queries, we'll want to do something more like your pull request using a window.location call.

courtney-rosenthal commented 10 years ago

I attempted a fix but got caught up turning out into a pull request. I'll take another shot, probably tomorrow.

On September 18, 2014 6:01:31 PM CDT, Andrew Vickers notifications@github.com wrote:

I already added a method of splitting queries for geolocation:

var URL = window.location.toString();

var queries = URL.split("?");
       if(queries[1] != undefined){
           var params = queries[1].split("&");
           var query = params[0].split("=");
           if(query[0] === "g" || "geo"){
               var GEOLOCATION = query[1];
           } 
       }

Your code is designed to handle multiple queries, where as I avoided handling that to keep the code base smaller, though your approach is very compact! The issue with your implementation here is that window.onpopstate is implemented differently across browsers. In Chrome and Safari, it will trigger on page load. In Firefox and IE, it will not. Thus, it is not a good production solution because geolocation will not work in all browsers if we go with this approach, and using both violates DRY.

We should only implement a window.onpopstate listener if we actually need it in production. If it is just going to be for testing, it would make more sense to use a test branch.

If we can envision a reason for multiple queries, we'll want to do something more like this using a window.location call.


Reply to this email directly or view it on GitHub: https://github.com/open-austin/voteatx-app/issues/2#issuecomment-56115569

Chip Rosenthal * 512-573-5174 * KE5VHV * http://www.unicom.com/ Austin #OpenGov and #CivicHacking events: http://www.open-austin.org/calendar