Overriding external link popup behaviour [#1592712]

Hi,

On a Drupal Commerce site I'm doing the client wants a popup to appear when leaving the site but only if the cart's not empty. I've been using your wonderful module for a while, and I thought it'd make sense to base my work on that. I have no problem overriding things server-side, but the JS is a bit more complicated (AFAIK I'd have to override all of extlinkAttach just to modify the few lines dealing with the popup message).

So I had a couple of questions:

Would you recommend against this for any reason?
Would you be able to modify the JS from

if (Drupal.settings.extlink.extAlert) {
  // Add pop-up click-through dialog.
  $(external_links).click(function(e) {
   return confirm(Drupal.settings.extlink.extAlertText);
  });
}

to something roughly along the lines of

$(external_links).click(function(e) {
  if (Drupal.extlink.showExtAlert()) {
    return confirm(Drupal.extlink.getExtAlertText());
  }
});

This gives a basic method for modules to alter the module's JS behaviour by overriding Drupal.extlink.showExtAlert() and Drupal.extlink.getExtAlertText(). (I'm thinking that a more powerful method isn't warranted here.)

If you're for this in principle, I'd be happy to provide a patch. Many thanks for any help/time you can spare.

Andy

Comment	File	Size	Author
#13	extlink-1592712-13.patch	1.1 KB	elachlan
#13
#4	extlink-overridable-popups-1592712-4.patch	1.09 KB	AndyF
#4
#3	extlink-overridable-popups-1592712-0.patch	1.05 KB	AndyF
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

quicksketch

he/him

CreditAttribution: quicksketch commented 22 May 2012 at 15:59

Rather than requiring a JavaScript override. I would suggest helping along #1054676: Exclude/include specific jQuery selectors from processing. And then if someone adds something to their cart, add a class to the body tag such as "cart-not-empty". Then you could add "body.cart-not-empty a" to the exclusion list. It requires a small amount of custom code on your part, but the configuration is more useful globally.

Comment #2

quicksketch

he/him

CreditAttribution: quicksketch commented 22 May 2012 at 17:19

Sorry I didn't read your post properly. I thought you were talking about the "open in new window" functionality, not the confirmation popup text. I think the adjustments you mentioned would probably be acceptable, though this is certainly an edge-case usage of the module. If you roll a patch I'll take a look at the implementation.

Comment #3

AndyF CreditAttribution: AndyF commented 23 May 2012 at 14:16

Status:

Active

» Needs review

File	Size
extlink-overridable-popups-1592712-0.patch	1.05 KB

Thanks so much! I also noticed your link to External Link Extras from the project page, so I've made it a bit more flexible to also hopefully cover their needs (currently they just unbind the click event from the links). With the patch it's a little less efficient than before in a couple of ways, but I'm hoping that's insignificant.

Hope this is ok - please let me know if there's anything else to be done.

Thanks

Comment #4

AndyF CreditAttribution: AndyF commented 23 May 2012 at 17:32

File	Size
extlink-overridable-popups-1592712-4.patch	1.09 KB

Slight change:

Drupal.extlink.popupClickHandler = function() {

becomes

Drupal.extlink.popupClickHandler = Drupal.extlink.popupClickHandler || function() {

which prevents extlink from clobbering a custom handler if its behaviors get attached again, and also allows overriding modules to declare the override before or after extlink's behaviors are attached.

I've got a sandbox module working with this.

Thanks again

Comment #5

justindodge CreditAttribution: justindodge commented 19 June 2012 at 16:17

Hey guys - maintainer of the extlink_extra module here.

Loving the patch!

Here's why: My method of extending the behavior of this module was simply to unbind the click handler on items that had the 'ext' class applied (as pointed out above) - but the problem with this is that I've recently discovered not all of the links that extlink attaches to have the 'ext' class added - image links being the example that I remember, and this is a pretty decent fail for extlink_extra.

I hadn't yet been able to come up with other ways that weren't grossly inefficient to overcome this, but this patch is an excellent solution.

Nice work! I haven't tested it yet, but would like to when I get a chance.

The only improvements that come to mind: If extending the functionality of extlink becomes a popular trend (and I could imagine a couple other cool things you could do with this - I've had some requests on my own issue queue for more complicated handling of link behaviors), I don't think this patch allows more than one module to interact with the click reaction, so you'd have compatibility problems with two modules trying to extend this handler.

I'm not sure if it's worth the energy to create a system that allows the registration of multiple handlers, but there's a thought for you anyway.

Comment #6

AndyF CreditAttribution: AndyF commented 20 June 2012 at 08:28

@justindodge Thanks for your input!

I'm not sure if it's worth the energy to create a system that allows the registration of multiple handlers, but there's a thought for you anyway.

I deliberately kept it simple to increase the chance that quicksketch would accept the patch. With the current version you can still store the old handler in a variable and call it from your new handler, which perhaps is enough to cover this need? If quicksketch is up for it we could use an array of handlers, or explicitly support multiple handlers some other way...

Comment #7

justindodge CreditAttribution: justindodge commented 6 August 2012 at 17:10

Andy I think the simplicity is a good first step, and in the interest of progress this patch gets my vote.

You are right in that there are a number of ways one could inject a handler without disrupting others, so there's no great impediment there.

Hopefully this gets committed so I can resolve the issue in my queue: #1715520: extlink_extra does not work with links that contain images.
:)

Comment #8

AndyF CreditAttribution: AndyF commented 6 August 2012 at 18:04

@justindodge sounds good to me. Would you be able to give the patch a once-over and check if it works ok for you with External Links Extra? That might help quicksketch when he finds time to return to this issue :)