Download & Extend

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

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

Comments

#1

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

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

Title:Danish map links» Danish map links (location.xx.inc location.dk.inc)

Can someone try this new patch?

#4

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

AttachmentSize
danish-location.patch 5.63 KB

#5

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

Status:needs review» fixed

Committed to DRUPAL-6--3 and HEAD.

#7

#8

Status:fixed» closed (fixed)

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

nobody click here