Problem/Motivation

To track the activities on an external link popup, it would be great to trigger events on any actions like,

  • Click on OK / Yes button
  • Click on Cancel / No button
  • No popup / Internal domain

This shall help in using the external link popup from other modules and make some programmatic decisions according to the event responses from the external link popup.

Steps to reproduce

  • Assuming I need to take some action on clicking Cancel button on External link popup.
  • Currently we are not able to figure out which button was clicked or whether the external popup is shown or not.

Proposed resolution

Trigger custom events for some important actions on the external link popup.

  • Clicking on OK button
  • No popup is shown (internal domain) or no popup is configured
  • Clicking on Cancel button / Escape key / (X) button will just close the popup, so the existing dialog:beforeclose should be good instead of raising a new event.

User interface changes

No user interface changes

CommentFileSizeAuthor
#12 external_link_popup-custom_events-3195990-12.patch5.13 KBdewalt
#11 external_link_popup-custom_events-3195990-11.patch5.01 KBdewalt
#8 external_link_popup-custom_events-3195990-8.patch4.94 KBdewalt
#6 custom-events-3195990-6.patch2.41 KBjamesashok
#2 custom-events-3195990-2.patch2.41 KBjamesashok

Comments

jamesashok created an issue. See original summary.

jamesashok’s picture

jamesashok commented
StatusFileSize
new custom-events-3195990-2.patch2.41 KB

Triggered custom event external-link:action,

  • Ok button click: action: yes
  • No popup shown: action: no-popup
jamesashok’s picture

jamesashok commented
Assigned: jamesashok » Unassigned
Status: Active » Needs review
dewalt’s picture

dewalt commented
Status: Needs review » Needs work

Thanks @jamesashok, it could be usefull. Comments I have:

+    const $window = $(window);

ES5 JS is used, it have no "const" keyword.

-        if (!popupId && (!domain || self.inDomain(domain, whitelist))) {
-          return;
-        }

It uses quick exit that makes code clear. I think it's better to trigger event here too and exit. And in this way the param could be used that shows - does popup not found or does the link whitelisted.

+        $window.trigger({
+          action: 'no-popup',
+          type: 'external-link:action',
+          url: element.href
+        });

I think action name in camel case would be better, e.g. "externalLinkPopupSkipped" or "extenalLinkPopup:skipped".

jamesashok’s picture

jamesashok commented
Assigned: Unassigned » jamesashok
jamesashok’s picture

jamesashok commented
StatusFileSize
new custom-events-3195990-6.patch2.41 KB

Thanks @dewalt

The suggested changes are in the updated patch. Please review.

jamesashok’s picture

jamesashok commented
Assigned: jamesashok » Unassigned
Status: Needs work » Needs review
dewalt’s picture

dewalt commented
StatusFileSize
new external_link_popup-custom_events-3195990-8.patch4.94 KB

@jamesashok, what do you see about this changes in implementation?

jamesashok’s picture

jamesashok commented

Sorry for the late reply @dewalt...

Just two comments/doubts on the above patch:-
- On dialog.js is it possible, that an anchor tag could have a data-external-link-popup-id attribute, valid external domain and a class .external-link-popup-disabled on it. If this scenario is possible, then we may execute anchor click code, when previously without the patch we would'nt have executed (as it is restricted by the :not(.external-link-popup-disabled))
- On README.txt, could we be more specific about the or a link class?

Rest of the patch is better, considering that we are covering more scenarios to raise the events...

dewalt’s picture

dewalt commented

Thanks, its possible, I'll fix it a bit later the same as README. If you'd like feel free to provide the fixes.

dewalt’s picture

dewalt commented
StatusFileSize
new external_link_popup-custom_events-3195990-11.patch5.01 KB

@jamesashok please look the fixed one

dewalt’s picture

dewalt commented
StatusFileSize
new external_link_popup-custom_events-3195990-12.patch5.13 KB

  • dewalt committed 66e2042 on 8.x-1.x authored by jamesashok 
    Issue #3195990 by dewalt, jamesashok: Events on Popup actions
dewalt’s picture

dewalt commented
Status: Needs review » Fixed

I've commited the changes into dev branch, I'll make additional check on holidays and make a release.

Status: Fixed » Closed (fixed)

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