I updated the function below to get "See Map" links in my nodes to open in a new window. Is there a better way to do this? Any chance it could be integrated into future versions as an admin option?

The key change is changing this:
$links[] = '<a href="'. $link .'">'. $providers[$mapper]['name'] .'</a>';

To this:
$links[] = '<a href="'. $link .'" target="_blank">'. $providers[$mapper]['name'] .'</a>';

function location_map_link($location = array(), $link_text = 'See map: ') {
  if (!isset($location['country']) && $location['country'] != 'xx') {
    return '';
  }

  $default_func = 'location_map_link_'. $location['country'] .'_default_providers';
  $providers_func = 'location_map_link_'. $location['country'] .'_providers';
  $providers = function_exists($providers_func) ? $providers_func() : array();
  $selected_providers = variable_get('location_map_link_'. $location['country'], function_exists($default_func) ? $default_func() : array());

  $links = array();
  foreach ($selected_providers as $mapper) {
    $link_func = 'location_map_link_'. $location['country'] .'_'. $mapper;
    if (function_exists($link_func)) {
      if ($link = $link_func($location)) {
        // ZXS
        $links[] = '<a href="'. $link .'" target="_blank">'. $providers[$mapper]['name'] .'</a>';
      }
    }
  }
  if (count($links)) {
    return t($link_text) . implode($links, ", ");
  }
  else {
    return NULL;
  }
}
CommentFileSizeAuthor
#14 location_maplink-20100415.patch1.77 KBhutch
#11 location-182786-11.patch1.45 KBfuerst

Comments

summit’s picture

summit commented

Hi,

May be off topic, but a code-question :)
Do you also know how to show the map, and not the link?
I would like to show the map itself on the node, and not the link TO the map...
Please help.

Thanks in advance,
greetings,
Martijn

JonSTeps’s picture

JonSTeps commented

You can do that by creating a gmap macro like the following

[gmap |id=block0 |zoom=10 |width=200px |height=200px |control=Small |type=Map]

that macro will also create a block which you can then place within your node. You could probably use the same code as above with a views argument to achieve the same result but i have never used that method. You can also change the variables in the above code to suit your needs.

mandclu’s picture

mandclu commented

+1 for this fix being committed.

yesct’s picture

yesct commented
Status: Active » Postponed

If the original poster, or someone wants to get this option in, please re-roll a patch that works against the latest version. I'll mark it postponed for now, but if someone wants to work on it, just change it back to active when you submit your code that works with the current versions.

yesct’s picture

yesct commented
Issue tags: +Location theming, +location theming map links

A patch that changes the code to add div tags might be more flexible. As that might allow people to theme it.

#172646: Add div class for map links is related.

Also, tagging.

Anonymous’s picture

Anonymous (not verified) commented
Title: open map links in new window » Open map links in new window
Version: 5.x-1.x-dev » 6.x-3.x-dev
Status: Postponed » Active

Using target="_blank" in the code is not ideal as it doesn't validate and is deprecated. A better solution would be to use jQuery to target specific links and open them externally that way.

This, however, requires the ability to add custom classes or tags to the <a> element. Therefore, we need a solution that allows the <a> element to be edited or added to. Possibly an overridable theme function, like theme_map_link...

Patches welcome!

mandclu’s picture

mandclu commented

What about using rel="external instead of target="_blank"?

Anonymous’s picture

Anonymous (not verified) commented

rel="external" is generally used as a way to target external links so they can then be edited with jQuery - it does nothing on its own (see here for the official definition).

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented

Target attribute is not deprecated in HTML5. Seems like theres a big work around just to make it XHTML 'valid'.

Dreher’s picture

Dreher commented

In what file are you making this change?

fuerst’s picture

fuerst commented
Status: Active » Needs review
StatusFileSize
new location-182786-11.patch1.45 KB

The attached patch adds a settings option to the Location module which allows entering the value of the target attribut in the <a> tag. Even if considered as not 100% W3C conform it just works for now (and me).

Patch tested with location-6.x-3.1-rc1 from 2009-Mar-03 but applies to location-6.x-3.x-dev from 2010-Mar-24 too.

yesct’s picture

yesct commented

can someone try duplicating the code in the patch to also allow entering a "rel" value? (see #11 and #7)
then people can pick which way they want to do it?

Also, someone needs to try this out to get it committed....

mr.andrey’s picture

mr.andrey commented

subscribing...

My two cents:

I don't see why the default behavior should be to open it in the same window to begin with. New window (target='blank') makes most sense for almost all setups.

Cheers,
Andrey.

hutch’s picture

hutch commented
StatusFileSize
new location_maplink-20100415.patch1.77 KB

The patch location-182786-11.patch in #11 goes in fine, however I needed to add a space before 'target' to make it work.

On the issue of rel="external"

w3c schools says:

The rel attribute is not supported in any of the major browsers.

Note: Browsers do not use this attribute in any way. However, search engines can use this attribute to get more information about a link.

As #6 says.

I consulted an SEO expert I work with and his view of the 'external' rel attribute was that it is not necessary since any html parser used by SEOs will know that it is an external link anyway.

Furthermore it does not appear at all in the official list of link types.

I tried rel="external" instead of target="_blank" and it did not work, as I would expect, it's the browser that decides, not jquery. If in the future this changes then this issue can be revisited.

target="_blank" may be 'depracated' but it works, now and that is more important it seems to me.

On comment #6, any classes that someone might want to add can be done using some jquery massage as part of the theme.

I would contend that this feature is a good idea but needs to be a checkbox which enables target="_blank"

The attached patch does this, on current cvs.

yesct’s picture

yesct commented

rock out. approach in #14 seems to make the most people happy! needs a review. :)

mandclu’s picture

mandclu commented

I think the absolute best approach would to have the module write rel="external" to the page, and then use jquery to add target="_blank" to all such links.

That said, I've also been done to add a global line of jquery that adds target="_blank" to all outside links which would make all of this unnecessary. :-P

I guess I'm just reinforcing the idea that there are many ways to solve this particular issue. Personally, I can't see any downside to having a checkbox to add the target property, but I would suggest that it default to being off, considering that it does break the validity of the output.

yesct’s picture

yesct commented

surge_martin has a good suggestion to have the checkbox off by default on the target blank property. Is that what is in the patch now?

I'm not sure looking at:

    '#default_value' => variable_get('location_maplink_external', 0),
    '#description' => t('Select this if you want the map link to open in a separate window'),
    '#return_value' => 1,

default value is 0, right? that means off?
and is the return value what the value is when it is checked?

hutch’s picture

hutch commented

It is off by default, so nothing changes unless you go in and set it. I wouldn't want anybody to have any unexpected surprises. That IMO is standard practise when adding a feature in admin/settings or anywhere else for that matter.

'#return_value' is what the form returns when the checkbox is checked, correct. This is also what gets put into the variable 'location_maplink_external' as you will see in the variable table when you inspect it with phpmyadmin.

@surge_martin, if you can provide code that will make rel="external" functional then perhaps we can work from that.

yesct’s picture

yesct commented

cool. (so just to summarize, #14 is the one to review at this point).

yesct’s picture

yesct commented
Issue tags: +location bdragon consider commit

tagging.

would be nice to get someone to test this though so it can be marked RTBC.

http://drupal.org/patch/review

fuerst’s picture

fuerst commented

Nice approach to just use a checkbox to trigger target=_blank. As Mac user I should have known this :)
Works, but tested with location-6.x-3.1-rc1 from 2009-Mar-03 only so not set to RTBC.

eporama’s picture

eporama commented
Status: Needs review » Reviewed & tested by the community

I set up a new D6 install, grabbed location-DRUPAL-6--3 (since HEAD is now apparently D7), allowed a location_node on the story content type with one location. Confirmed that links open in the same window. Applied patch in #14 with no warnings. Flushed caches; new checkbox on admin/settings/location/ (disabled by default), when checked, confirmed that target="_blank" was added to the maplink a element and that the map did indeed open in a new window.

Marking as RTBC

yesct’s picture

yesct commented

eporama great review!

rooby’s picture

rooby commented
Status: Reviewed & tested by the community » Fixed

Committed to D5, D6 and HEAD. Thanks for the patch.

I added one more option that allows the admins to select whether to use target="_blank" or rel="external".
jQuery is left up to the user so selecting rel="external" will do nothing until that is added to the theme.

That should keep everyone happy :)

yesct’s picture

yesct commented
Issue tags: -location bdragon consider commit

http://drupal.org/cvs?commit=361006
http://drupal.org/cvs?commit=361008
http://drupal.org/cvs?commit=361010

Awesome! Thanks rooby!!!!

Status: Fixed » Closed (fixed)

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

rooby’s picture

rooby commented

Just realised I committed to the DRUPAL-5 branch instead of the DRUPAL-5--3 branch :(

Committed to DRUPAL-5--3 - http://drupal.org/cvs?commit=372104