We need to have some code run when the popup is opened or closed, so patch to follow that will trigger an event at the end of the relevant animations (our particular case is for visual purposes so it needs to be after the animation).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

stevetweeddale’s picture

Patch attached. A small amount of refactoring was required to avoid having to trigger the events in multiple code paths.

svenryen’s picture

Status: Active » Needs review
FileSize
2.1 KB

Hi @stevetweeddale! A lot has changed since you contributed your patch. I tried to implement the same behavior in the latest -dev. Can you have a look at the attached patch and see whether the events trigger correctly?

svenryen’s picture

I had a problem with the variable names. Ignore #2, here's a patch that should work properly.

svenryen’s picture

I tested the triggers and found that they all fired, so I'm adding this to -dev. Thanks, stevetweeddale!

svenryen’s picture

Status: Needs review » Fixed

  • svenryen committed aa984ad on 7.x-2.x
    Merge branch '7.x-1.x' into 7.x-2.x
    
    * 7.x-1.x: (76 commits)
    Fixed an...
  • svenryen committed c928f53 on 7.x-2.x authored by stevetweeddale
    Issue #2423649 by svenryen, stevetweeddale: Add event for popup open/...

Status: Fixed » Closed (fixed)

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

jcmiller09’s picture

For the lines removing the popup and triggering the custom event
$('#sliding-popup').remove().trigger('eu_cookie_compliance_popup_close');
the remove() method is happening first and not triggering the event since everything is dumped from that element. Putting the trigger() before the remove() allows the event to fire.

jcmiller09’s picture

jcmiller09’s picture

jcmiller09’s picture

It might also be better to move the event triggers to before the .animate() calls, in case one needs to synchronize other elements on the page. The way it fires now, it has to wait for the animation to complete before the callback fires.

svenryen’s picture

Status: Closed (fixed) » Needs review

Nice catch, @jcmiller09 ! I'll take a look at the patch!

  • svenryen committed 571ef90 on 7.x-1.x authored by jcmiller09
    Issue #2423649 by svenryen, jcmiller09, stevetweeddale: Add event for...

  • svenryen committed 42910fb on 8.x-1.x authored by jcmiller09
    Issue #2423649 by svenryen, jcmiller09, stevetweeddale: Add event for...
svenryen’s picture

Status: Needs review » Fixed

  • svenryen committed 571ef90 on 7.x-2.x authored by jcmiller09
    Issue #2423649 by svenryen, jcmiller09, stevetweeddale: Add event for...
  • svenryen committed 884a355 on 7.x-2.x
    Merge branch '7.x-1.x' into 7.x-2.x
    
    * 7.x-1.x:
    Issue #2975382 by...

Status: Fixed » Closed (fixed)

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