-
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
Changes from all commits
e74fade
12b3329
fe2f6da
c3f4c3d
04bc895
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,8 +4,10 @@ | |
module.exports = Marker; | ||
|
||
var DOM = require('../util/dom'); | ||
var util = require('../util/util'); | ||
var LngLat = require('../geo/lng_lat'); | ||
var Point = require('point-geometry'); | ||
var Popup = require('./popup'); | ||
|
||
/** | ||
* Creates a marker component | ||
|
@@ -36,7 +38,7 @@ Marker.prototype = { | |
* @param {Map} map | ||
* @returns {Marker} `this` | ||
*/ | ||
addTo: function(map) { | ||
addTo: function (map) { | ||
this.remove(); | ||
this._map = map; | ||
map.getCanvasContainer().appendChild(this._el); | ||
|
@@ -52,21 +54,22 @@ Marker.prototype = { | |
* marker.remove(); | ||
* @returns {Marker} `this` | ||
*/ | ||
remove: function() { | ||
remove: function () { | ||
if (this._map) { | ||
this._map.off('move', this._update); | ||
this._map = null; | ||
} | ||
var parent = this._el.parentNode; | ||
if (parent) parent.removeChild(this._el); | ||
if (this._popup) this._closePopup(); | ||
return this; | ||
}, | ||
|
||
/** | ||
* Get the marker's geographical location | ||
* @returns {LngLat} | ||
*/ | ||
getLngLat: function() { | ||
getLngLat: function () { | ||
return this._lngLat; | ||
}, | ||
|
||
|
@@ -75,19 +78,91 @@ Marker.prototype = { | |
* @param {LngLat} lnglat | ||
* @returns {Marker} `this` | ||
*/ | ||
setLngLat: function(lnglat) { | ||
setLngLat: function (lnglat) { | ||
this._lngLat = LngLat.convert(lnglat); | ||
if (this._popup) this._popup.setLngLat(this._lngLat); | ||
this._update(); | ||
return this; | ||
}, | ||
|
||
getElement: function() { | ||
getElement: function () { | ||
return this._el; | ||
}, | ||
|
||
_update: function() { | ||
/** | ||
* Binds a Popup to the Marker | ||
* @param {Popup=} popup an instance of the `Popup` class. If undefined or null, any popup | ||
* set on this `Marker` instance is unset | ||
* @returns {Marker} `this` | ||
*/ | ||
|
||
setPopup: function (popup) { | ||
if (popup == null) { | ||
this._closePopup(); | ||
delete this._popupHandlersAdded; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of tracking state in |
||
delete this._popup; | ||
} else if (popup instanceof Popup) { | ||
this._popup = popup; | ||
} else { | ||
util.warnOnce('Marker.setPopup only accepts an instance of the Popup class as an argument. If no argument is provided, the popup is unset from this Marker instance'); | ||
} | ||
|
||
if (this._popup && this._lngLat) this._popup.setLngLat(this._lngLat); | ||
|
||
if (!this._popupHandlersAdded) { | ||
this.getElement().addEventListener('click', this._openPopup.bind(this)); | ||
this._popupHandlersAdded = true; | ||
} | ||
return this; | ||
}, | ||
|
||
/** | ||
* Returns the Popup instance that is bound to the Marker | ||
* @returns {Popup} popup | ||
*/ | ||
getPopup: function () { | ||
return this._popup; | ||
}, | ||
|
||
/** | ||
* Opens or closes the bound popup, depending on the current state | ||
* @returns {Marker} `this` | ||
*/ | ||
togglePopup: function () { | ||
if (this._popup) { | ||
if (this._popup._map) { | ||
this._closePopup(); | ||
} else { | ||
this._openPopup(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will throw an error because |
||
} | ||
} | ||
}, | ||
|
||
_openPopup: function (e) { | ||
// prevent event from bubbling up to the map canvas | ||
e.stopPropagation(); | ||
|
||
if (!this._popup || !this._map) return; | ||
|
||
if (!this._popup._map) { | ||
this._popup.addTo(this._map); | ||
} | ||
|
||
return this; | ||
}, | ||
|
||
_closePopup: function () { | ||
if (this._popup) { | ||
this._popup.remove(); | ||
} | ||
|
||
return this; | ||
}, | ||
|
||
_update: function () { | ||
if (!this._map) return; | ||
var pos = this._map.project(this._lngLat)._add(this._offset); | ||
DOM.setTransform(this._el, 'translate(' + pos.x + 'px,' + pos.y + 'px)'); | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
'use strict'; | ||
|
||
var test = require('tap').test; | ||
var jsdomGlobal = require('jsdom-global'); | ||
|
||
var extend = require('../../../js/util/util').extend; | ||
var Map = require('../../../js/ui/map'); | ||
var Marker = require('../../../js/ui/marker'); | ||
var Popup = require('../../../js/ui/popup'); | ||
|
||
function prepareDOM() { | ||
var cleanup = jsdomGlobal(); | ||
var mapdiv = document.createElement('div'); | ||
mapdiv.id = "map"; | ||
document.body.appendChild(mapdiv); | ||
return cleanup; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you refactor this so that we don't need to use global variables? You should be able to do everything you need with access to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was having issues with the functions run by the tests failing because document + window weren't in scope. I refactored to use the Otherwise we can take the approach that |
||
|
||
function createMap(options, callback) { | ||
var map = new Map(extend({ | ||
container: 'map', | ||
attributionControl: false, | ||
trackResize: true, | ||
style: { | ||
"version": 8, | ||
"sources": {}, | ||
"layers": [] | ||
} | ||
}, options)); | ||
|
||
if (callback) map.on('load', function () { | ||
callback(null, map); | ||
}); | ||
|
||
return map; | ||
} | ||
|
||
|
||
|
||
test('Marker', function (t) { | ||
t.test('constructor', function (t) { | ||
var cleanup = prepareDOM(); | ||
var el = document.createElement('div'); | ||
var marker = new Marker(el); | ||
t.ok(marker.getElement(), 'marker element is created'); | ||
cleanup(); | ||
t.end(); | ||
}); | ||
|
||
t.test('marker is added to map', function (t) { | ||
var cleanup = prepareDOM(); | ||
var map = createMap(); | ||
var el = document.createElement('div'); | ||
map.on('load', function () { | ||
var marker = new Marker(el).setLngLat([-77.01866, 38.888]); | ||
t.ok(marker.addTo(map) instanceof Marker, 'marker.addTo(map) returns Marker instance'); | ||
t.ok(marker._map, 'marker instance is bound to map instance'); | ||
cleanup(); | ||
t.end(); | ||
}); | ||
}); | ||
|
||
t.test('popups can be bound to marker instance', function (t) { | ||
var cleanup = prepareDOM(); | ||
var map = createMap(); | ||
var el = document.createElement('div'); | ||
var popup = new Popup(); | ||
var marker = new Marker(el).setLngLat([-77.01866, 38.888]).addTo(map); | ||
marker.setPopup(popup); | ||
t.ok(marker.getPopup() instanceof Popup, 'popup created with Popup instance'); | ||
cleanup(); | ||
t.end(); | ||
}); | ||
|
||
t.test('popups can be unbound from a marker instance', function (t) { | ||
var cleanup = prepareDOM(); | ||
var map = createMap(); | ||
var el = document.createElement('div'); | ||
var marker = new Marker(el).setLngLat([-77.01866, 38.888]).addTo(map); | ||
marker.setPopup(new Popup()); | ||
t.ok(marker.getPopup() instanceof Popup); | ||
t.ok(marker.setPopup() instanceof Marker, 'passing no argument to Marker.setPopup() is valid'); | ||
t.ok(!marker.getPopup(), 'Calling setPopup with no argument successfully removes Popup instance from Marker instance'); | ||
t.end(); | ||
cleanup(); | ||
}); | ||
|
||
t.end(); | ||
}); | ||
|
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.
this._closePopup
has an internalif (this._popup)
guard -- either call it unguarded here, or add external guards everywhere it's called and remove the internal guard.