-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add bindPopup API to the Marker class #3056
Conversation
* @returns {Marker} `this` | ||
*/ | ||
|
||
bindPopup: function(content, options){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make Popup
the only type accepted by bindPopup
, and remove options
. This separates concerns:
Popup
is responsible for construction and configuration of the popup.bindPopup
is responsible for linking it with aMarker
. It doesn't need to pass through all functionality provided byPopup
.
@lucaswoj copying from my response to your 'outdated' line comment: Otherwise we can take the approach that |
setPopup: function (popup) { | ||
if (popup == null) { | ||
this._closePopup(); | ||
delete this._popupHandlersAdded; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of tracking state in _popupHandlersAdded
, callremoveEventListener
when the popup is removed. This will prevent memory leaks and unexpected behavior if the Marker
continues to be used without a popup. You'll need to save the this
-bound version of _openPopup
, say by using util.bindAll(['_openPopup'], this)
, so that it can be passed to removeEventListener
.
* Fix exceptions thrown when opening marker-bound popup by click Resolves #3056 (comment) * Remove unnecessary guard clause * Remove private popup property access * Refactor marker popup binding * Put markers in the "control container" * Ensure "click" event is fired on "map" from a marker * Attach marker click listener to map to avoid setTimeout
This PR introduces methods for using
Popup
s in conjunction withMarker
s and is not-so-loosely based on Leaflet's API of the same name.Also includes the first DOM tests in the gl-js unit test suite using
jsdom
. I had to make some changes tojs/util/dom
in order for the tests to complete without errors, but there might be other workarounds using jsdom that I'm not aware of.Provides a fix for the problem of adding a
Popup
on aclick
event to aMarker
with the current API -- the Popup is created and then immediately destroyed unless you havePopup.closeOnClick: false
because the click event bubbles up to the map canvas. Related discussion at #2931Checklist
master