-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Make callouts not close the reveal they are in. #8471
Conversation
This solution is kinda sloppy, maybe there is a more elegant way to solve this...
This will fix #8428, so many thanks for fixing this! |
Hmm... this approach feels like it could have unintended side effects. What if instead we have the callout event handler call event.stopPropagation() to prevent it bubbling up? |
Well there is no callout event handler because it's all done in the (document).on('close.zf.trigger', '[data-closable]', function(e){
e.stopPropagation();
let animation = $(this).data('closable');
if(animation !== ''){
Foundation.Motion.animateOut($(this), animation, function() {
$(this).trigger('closed.zf');
});
}else{
$(this).fadeOut().trigger('closed.zf');
}
}); There is already a |
Ah interesting... the reveal handler is probably getting called first, given that the callout doesn't have its own handler but is depending on that base handler. Hmm... so I still don't think this is the right fix, because it will break the ability to simply include a 'data-close' element inside a reveal... but I'm not sure what the right fix is. I'm also not 100% sure that element is going to get passed along in all click cases... What we're really trying to do is handle nesting... so what if instead of looking at element we did a check something like this
Basically looking to see if there's another data-closable element in the tree before you get to the reveal event handler? |
@Owlbertz can you take a look at the concern above and rework this fix to still allow using data-close inside of a reveal? |
Also added visual test.
Great. Nice work! |
This solution is kinda sloppy, maybe there is a more elegant way to solve this...
Hope someone else can review this soon, really would like to see this in the upcoming release.