Fix for dismissible option breaking with multiple popovers open #250
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sbrodkey
commented
Sep 6, 2017
| @@ -78,7 +78,6 @@ | |||
| var _srcElements = []; | |||
| var backdrop = $('<div class="webui-popover-backdrop"></div>'); | |||
| var _globalIdSeed = 0; | |||
| var _isBodyEventHandled = false; | |||
There was a problem hiding this comment.
This global setting was preventing the bodyClickHandler from being able to bind multiple times. It would only allow one bind. We want as many binds as there are popovers to happen.
sbrodkey
commented
Sep 6, 2017
| @@ -727,19 +726,18 @@ | |||
| }, | |||
|
|
|||
| bindBodyEvents: function() { | |||
| if (_isBodyEventHandled) { | |||
| return; | |||
| } | |||
| if (this.options.dismissible && this.getTrigger() === 'click') { | |||
| if (isMobile) { | |||
| $document.off('touchstart.webui-popover').on('touchstart.webui-popover', $.proxy(this.bodyTouchStartHandler, this)); | |||
There was a problem hiding this comment.
Haven't tested on mobile, but if we like the separation this provides in the lines below, we could add a unique id here as well.
q2apro
pushed a commit
to q2apro/webui-popover
that referenced
this pull request
Aug 15, 2019
sandywalker#250 Samuel Brodkey on Sep 6, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been working on an app where many popovers are present, using a mix of multi:true and dismissible:false and have run into issues where the dismissible option seems to not be functioning correctly.
Problem: Opening a dismissible popover along with a non-dimissible popover can cause the non-dismissible popover to become dismissible! This is because when we have the non-dismissible popover open and click outside of it, the "bodyClickHandler" runs for the dismissible popover and calls hideAllPop() inside of it! We would prefer that it only calls this.hide() to only hide itself, not other non-dismissible popovers.
So, we have now made it so popovers can only dismiss themselves. I think this makes sense as a dismissible popover should not be able to tell a non-dismissible popover to close (unless multi:false).
However, this reveals a pre-existing issue in that not all bodyClickHandlers are running for every popover. If we click outside of two dismissible popovers, only one will close, showing that not all are running. This is because "click.webui-popover" is not a unique event. To make it unique, we append this._idSeed. Now, each popover has its own bodyClickHandler in which it can close itself if the right conditions occur.
Please see demo/test-issue248.html which I've created and make sure we're running the version of web-uipopover before these changes! It will walk you through these issues and demonstrate why these fixes are necessary.