Download & Extend
Location » Issues

Danish map links (location.xx.inc location.dk.inc)

Posted by mikl on March 24, 2009 at 1:56pm
Project:Location
Version:6.x-3.x-dev
Component:Data update
Category:task
Priority:normal
Assigned:mikl
Status:closed (fixed)

Issue Summary

I've expanded location.dk.inc with map links to the Danish map site, findvej.dk.

Patch attached :)

AttachmentSize
dk.location.patch3.23 KB
Login or register to post comments

Comments

#1

Posted by bdragon on March 24, 2009 at 5:45pm

Some nits:

* Don't use sprintf. Concatenation is fine. Using url() to do the heavy lifting is best.
* findvej.dk seems to be fine with using just a postcode, there should be a case for that, no?
* I'd prefer lat/lon over the rest of the choices, rather than the other way around. This makes it easier on the geocoder.
* You should use location_has_coordinates() to check whether the location has latitude / longitude set. This catches corner cases like stuff set to "0.0000" and so forth.
* You're checking if the fields are set, but you aren't checking if they are empty or not when determining the link.
* The raw ampersand when you're putting together the lat/lon url is suspect. In HTML this must be changed to the & entity.
* You accidentally joined the cvs id line with the top line of the file. (I automatically fix stuff like this when testing patches, so it's not actually a problem as such.)

#2

Posted by mikl on March 25, 2009 at 11:27am

Okay, re-roll:

  • Okay, sprintf might be overkill here, even if it makes the code a bit cleaner…
  • While postcodes are supported, Google's geocoding of postcodes in Denmark is a bit clueless and provides unexpected results. See for example my old hometown, Snedsted: http://findvej.dk/7752 – the city marker is placed well outside the city, half a mile away from the city center.
  • While using coordinates are cleaner and saves a geocoding-lookup up front, findvej will then not have the address available, so it won't be displayed on the map, and if you use any of findvej's features, it'll get the address via a reverse geocode lookup, so we won't be saving many calls and providing a poorer user experience.
  • I stand corrected, now using location_has_coordinates().
  • Corrected field checking as well.
  • Yeah, I've converted the code to use url()
  • I've changed it back to the old style.
AttachmentSize
location.dk_.inc_.patch 3.2 KB

#3

Posted by YesCT on April 17, 2009 at 2:11pm
Title:Danish map links» Danish map links (location.xx.inc location.dk.inc)

Can someone try this new patch?

#4

Posted by mikl on June 1, 2009 at 1:11pm

I've made a new patch with support for the Danish public transportation guide, Rejseplanen.dk :)

AttachmentSize
danish-location.patch 5.63 KB

#5

Posted by kaerast on June 25, 2009 at 11:44am

This latest patch isn't applying for me for some reason. I therefore attach my own version which I rolled based upon yours.

AttachmentSize
location.dk_.inc_.diff 4.16 KB

#6

Posted by ankur on March 28, 2010 at 10:59pm
Status:needs review» fixed

Committed to DRUPAL-6--3 and HEAD.

#7

Posted by YesCT on April 11, 2010 at 6:02am

http://drupal.org/cvs?commit=347480

#8

Posted by System Message on April 25, 2010 at 6:10am
Status:fixed» closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

nobody click here