From 7cdafc7aa5c3d49b228c3eaddd356ee21f90123b Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 09:52:30 -0700 Subject: [PATCH 01/13] Add failing test --- test/test.geocoder.js | 85 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 83 insertions(+), 2 deletions(-) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index 86cd6409..4e31024f 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -828,7 +828,7 @@ test('geocoder', function(tt) { t.end(); }); - tt.test('options.marker [true]', function(t) { + tt.test('options.marker [true] when querying', function(t) { t.plan(2); setup({ @@ -849,6 +849,49 @@ test('geocoder', function(tt) { ); }); + tt.test('options.marker [true] when geolocating', function(t) { + t.plan(4); + + t.teardown(function() { + sinon.restore(); + }); + + setup({ + marker: true, + mapboxgl: mapboxgl + }); + + const geolocationPositionStub = { + coords: { + accuracy: 10, + altitude: null, + altitudeAccuracy: null, + heading: null, + latitude: 38.8999242, + longitude: -77.0361813, + speed: null + }, + timestamp: new Date('2022-01-01T00:00:00Z') + }; + sinon.replace(geocoder.geolocation, 'getCurrentPosition', + sinon.fake.resolves(geolocationPositionStub)); + + const markerConstructorSpy = sinon.spy(mapboxgl, 'Marker'); + + geocoder._geolocateUser(); + + geocoder.on( + 'result', + once(function(event) { + t.ok(markerConstructorSpy.calledOnce, 'a new marker is added to the map'); + const calledWithOptions = markerConstructorSpy.args[0][0]; + t.equals(calledWithOptions.color, '#4668F2', 'a default color is set'); + t.equals(geolocationPositionStub.coords.latitude, event.result.user_coordinates[1], 'the marker is placed at the correct latitude'); + t.equals(geolocationPositionStub.coords.longitude, event.result.user_coordinates[0], 'the marker is placed at the correct longitude'); + }) + ); + }); + tt.test('options.marker [constructor properties]', function(t) { t.plan(4); @@ -876,7 +919,7 @@ test('geocoder', function(tt) { ); }); - tt.test('options.marker [false]', function(t) { + tt.test('options.marker [false] when querying', function(t) { t.plan(1); setup({ @@ -894,6 +937,44 @@ test('geocoder', function(tt) { ); }); + tt.test('options.marker [false] when geolocating', function(t) { + t.plan(1); + + t.teardown(function() { + sinon.restore(); + }); + + setup({ + marker: false + }); + + const geolocationPositionStub = { + coords: { + accuracy: 10, + altitude: null, + altitudeAccuracy: null, + heading: null, + latitude: 38.8999242, + longitude: -77.0361813, + speed: null + }, + timestamp: new Date('2022-01-01T00:00:00Z') + }; + sinon.replace(geocoder.geolocation, 'getCurrentPosition', + sinon.fake.resolves(geolocationPositionStub)); + + const markerConstructorSpy = sinon.spy(mapboxgl, 'Marker'); + + geocoder._geolocateUser(); + + geocoder.on( + 'result', + once(function() { + t.ok(markerConstructorSpy.notCalled, 'a new marker is not added to the map'); + }) + ); + }); + tt.test('geocode#onRemove', function(t){ setup({marker: true}); From 62edf1fcc2c97608e2b3231130d913ed5d1757e2 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 09:55:50 -0700 Subject: [PATCH 02/13] DRY up geolocation stubbing --- test/test.geocoder.js | 52 ++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 30 deletions(-) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index 4e31024f..52fb4d44 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -26,6 +26,26 @@ test('geocoder', function(tt) { map.addControl(geocoder); } + function stubGeolocationPosition() { + const geolocationPositionStub = { + coords: { + accuracy: 10, + altitude: null, + altitudeAccuracy: null, + heading: null, + latitude: 38.8999242, + longitude: -77.0361813, + speed: null + }, + timestamp: new Date('2022-01-01T00:00:00Z') + }; + + sinon.replace(geocoder.geolocation, 'getCurrentPosition', + sinon.fake.resolves(geolocationPositionStub)); + + return geolocationPositionStub; + } + tt.test('initialized', function(t) { setup(); t.ok(geocoder, 'geocoder is initialized'); @@ -861,21 +881,7 @@ test('geocoder', function(tt) { mapboxgl: mapboxgl }); - const geolocationPositionStub = { - coords: { - accuracy: 10, - altitude: null, - altitudeAccuracy: null, - heading: null, - latitude: 38.8999242, - longitude: -77.0361813, - speed: null - }, - timestamp: new Date('2022-01-01T00:00:00Z') - }; - sinon.replace(geocoder.geolocation, 'getCurrentPosition', - sinon.fake.resolves(geolocationPositionStub)); - + const geolocationPositionStub = stubGeolocationPosition(); const markerConstructorSpy = sinon.spy(mapboxgl, 'Marker'); geocoder._geolocateUser(); @@ -948,21 +954,7 @@ test('geocoder', function(tt) { marker: false }); - const geolocationPositionStub = { - coords: { - accuracy: 10, - altitude: null, - altitudeAccuracy: null, - heading: null, - latitude: 38.8999242, - longitude: -77.0361813, - speed: null - }, - timestamp: new Date('2022-01-01T00:00:00Z') - }; - sinon.replace(geocoder.geolocation, 'getCurrentPosition', - sinon.fake.resolves(geolocationPositionStub)); - + stubGeolocationPosition(); const markerConstructorSpy = sinon.spy(mapboxgl, 'Marker'); geocoder._geolocateUser(); From d689aec3981e2b99db10aad8d4774b7f753ef3bf Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 09:59:18 -0700 Subject: [PATCH 03/13] Make failing test pass --- lib/index.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/index.js b/lib/index.js index c182ca8d..6a4105e6 100644 --- a/lib/index.js +++ b/lib/index.js @@ -292,7 +292,7 @@ MapboxGeocoder.prototype = { footerNode.addEventListener('mousedown', function() { this.selectingListItem = true; }.bind(this)); - + footerNode.addEventListener('mouseup', function() { this.selectingListItem = false; }.bind(this)); @@ -331,7 +331,10 @@ MapboxGeocoder.prototype = { } }; - this._handleMarker(geojson); + if (this.options.marker && this._mapboxgl) { + this._handleMarker(geojson); + } + this._fly(geojson); this._typeahead.clear(); @@ -355,11 +358,11 @@ MapboxGeocoder.prototype = { } else { this.geocoderService.reverseGeocode(config).send().then(function (resp) { const feature = resp.body.features[0]; - + if (feature) { const locationText = utils.transformFeatureToGeolocationText(feature, this.options.addressAccuracy); this._setInputValue(locationText); - + feature.user_coordinates = geojson.geometry.coordinates; this._eventEmitter.emit('result', { result: feature }); } else { @@ -407,7 +410,7 @@ MapboxGeocoder.prototype = { _setInputValue: function (value) { this._inputEl.value = value; - + setTimeout(function () { this._inputEl.focus(); this._inputEl.scrollLeft = 0; @@ -487,7 +490,7 @@ MapboxGeocoder.prototype = { _showLoadingIcon: function() { this._loadingEl.style.display = 'block'; }, - + _hideLoadingIcon: function() { this._loadingEl.style.display = 'none'; }, @@ -495,7 +498,7 @@ MapboxGeocoder.prototype = { _showAttribution: function() { this._footerNode.style.display = 'block' }, - + _hideAttribution: function() { this._footerNode.style.display = 'none' }, From 6da1490187b68c577b179dff536ba09a2cea88f6 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:05:30 -0700 Subject: [PATCH 04/13] DRY up marker option check --- lib/index.js | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/index.js b/lib/index.js index 6a4105e6..fc885142 100644 --- a/lib/index.js +++ b/lib/index.js @@ -331,7 +331,7 @@ MapboxGeocoder.prototype = { } }; - if (this.options.marker && this._mapboxgl) { + if (this._shouldAddMarker()) { this._handleMarker(geojson); } @@ -518,7 +518,8 @@ MapboxGeocoder.prototype = { if (this.options.flyTo) { this._fly(selected); } - if (this.options.marker && this._mapboxgl){ + + if (this._shouldAddMarker()) { this._handleMarker(selected); } @@ -1314,6 +1315,15 @@ MapboxGeocoder.prototype = { return this; }, + /** + * Determine whether a marker should be added to the map. + * @returns {Boolean} `true` if a marker should be added, `false` if not + * @private + */ + _shouldAddMarker: function() { + return this.options.marker && this._mapboxgl; + }, + /** * Handle the removal of a result marker * @private From e5f747146ccca5c913c3be6a6efb6e953d8a9073 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:41:05 -0700 Subject: [PATCH 05/13] Respect options.flyTo when geolocating --- lib/index.js | 17 +++++++- test/test.geocoder.js | 97 +++++++++++++++++++++++++++++++++++++------ 2 files changed, 100 insertions(+), 14 deletions(-) diff --git a/lib/index.js b/lib/index.js index fc885142..3e4f6343 100644 --- a/lib/index.js +++ b/lib/index.js @@ -335,7 +335,9 @@ MapboxGeocoder.prototype = { this._handleMarker(geojson); } - this._fly(geojson); + if (this._shouldFly()) { + this._fly(geojson); + } this._typeahead.clear(); this._typeahead.selected = true; @@ -511,11 +513,13 @@ MapboxGeocoder.prototype = { this._collapse(); } }, + _onChange: function() { var selected = this._typeahead.selected; if (selected && JSON.stringify(selected) !== this.lastSelected) { this._hideClearButton(); - if (this.options.flyTo) { + + if (this._shouldFly()) { this._fly(selected); } @@ -534,6 +538,15 @@ MapboxGeocoder.prototype = { } }, + /** + * Determines whether the map should fly to a new location after geocoding. + * @returns {Boolean} `true` if the map should fly to new locations, `false` if not + * @private + */ + _shouldFly: function() { + return this.options.flyTo; + }, + _fly: function(selected) { var flyOptions; if (selected.properties && exceptions[selected.properties.short_code]) { diff --git a/test/test.geocoder.js b/test/test.geocoder.js index 52fb4d44..120c7d9a 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -737,39 +737,108 @@ test('geocoder', function(tt) { t.end() }); - tt.test('options.flyTo [false]', function(t){ + tt.test('options.flyTo [false] when querying', function(t){ t.plan(1) setup({ flyTo: false }); - var mapFlyMethod = sinon.spy(map, "flyTo"); + const mapFlySpy = sinon.spy(map, 'flyTo'); + geocoder.query('Golden Gate Bridge'); + geocoder.on( 'result', once(function() { - t.ok(mapFlyMethod.notCalled, "The map flyTo was not called when the option was set to false") + t.ok(mapFlySpy.notCalled, 'flyTo() was not called after querying'); }) ); }); + tt.test('options.flyTo [false] when geolocating', function(t) { + t.plan(1); + + t.teardown(function() { + sinon.restore(); + }); + + setup({ + flyTo: false + }); + + stubGeolocationPosition(); + const flyToSpy = sinon.spy(map, 'flyTo'); - tt.test('options.flyTo [true]', function(t){ + geocoder._geolocateUser(); + + geocoder.on( + 'result', + once(function() { + t.ok(flyToSpy.notCalled, 'flyTo() was not called after geolocating'); + }) + ); + }); + + tt.test('options.flyTo [true] when querying', function(t){ t.plan(4) setup({ flyTo: true }); - var mapFlyMethod = sinon.spy(map, "flyTo"); + const flyToSpy = sinon.spy(map, "flyTo"); + geocoder.query('Golden Gate Bridge'); + geocoder.on( 'result', once(function() { - t.ok(mapFlyMethod.calledOnce, "The map flyTo was called when the option was set to true"); - var calledWithArgs = mapFlyMethod.args[0][0]; - t.equals(+calledWithArgs.center[0].toFixed(4), +-122.4809.toFixed(4), 'the map is directed to fly to the right longitude'); - t.equals(+calledWithArgs.center[1].toFixed(4), +37.8181.toFixed(4), 'the map is directed to fly to the right latitude'); - t.deepEqual(calledWithArgs.zoom, 16, 'the map is directed to fly to the right zoom'); + t.ok(flyToSpy.calledOnce, 'flyTo() is called after querying'); + + const calledWithArgs = flyToSpy.args[0][0]; + + t.equals(+calledWithArgs.center[0].toFixed(4), + +-122.4809.toFixed(4), + 'the map is directed to fly to the right longitude'); + + t.equals(+calledWithArgs.center[1].toFixed(4), + +37.8181.toFixed(4), + 'the map is directed to fly to the right latitude'); + + t.deepEqual(calledWithArgs.zoom, 16, + 'the map is directed to fly to the right zoom level'); + }) + ); + }); + + tt.test('options.flyTo [true] when geolocating', function(t) { + t.plan(4); + + t.teardown(function() { + sinon.restore(); + }); + + setup({ + flyTo: true + }); + + const geolocationPositionStub = stubGeolocationPosition(); + const flyToSpy = sinon.spy(map, 'flyTo'); + + geocoder._geolocateUser(); + + geocoder.on( + 'result', + once(function() { + t.ok(flyToSpy.calledOnce, 'flyTo() is called after geolocating'); + const calledWithArgs = flyToSpy.args[0][0]; + t.equals(+calledWithArgs.center[0].toFixed(4), + +geolocationPositionStub.coords.longitude.toFixed(4), + 'the map is directed to fly to the right longitude'); + t.equals(+calledWithArgs.center[1].toFixed(4), + +geolocationPositionStub.coords.latitude.toFixed(4), + 'the map is directed to fly to the right latitude'); + t.deepEqual(calledWithArgs.zoom, 16, + 'the map is directed to fly to the right zoom level'); }) ); }); @@ -892,8 +961,12 @@ test('geocoder', function(tt) { t.ok(markerConstructorSpy.calledOnce, 'a new marker is added to the map'); const calledWithOptions = markerConstructorSpy.args[0][0]; t.equals(calledWithOptions.color, '#4668F2', 'a default color is set'); - t.equals(geolocationPositionStub.coords.latitude, event.result.user_coordinates[1], 'the marker is placed at the correct latitude'); - t.equals(geolocationPositionStub.coords.longitude, event.result.user_coordinates[0], 'the marker is placed at the correct longitude'); + t.equals(+event.result.user_coordinates[1].toFixed(4), + +geolocationPositionStub.coords.latitude.toFixed(4), + 'the marker is placed at the correct latitude'); + t.equals(+event.result.user_coordinates[0].toFixed(4), + +geolocationPositionStub.coords.longitude.toFixed(4), + 'the marker is placed at the correct longitude'); }) ); }); From 7d298dd03a4fd63d7d83f4a0b8c479a2ee37c31f Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:43:09 -0700 Subject: [PATCH 06/13] Reposition _shouldAddMarker() --- lib/index.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/lib/index.js b/lib/index.js index 3e4f6343..69ea6c8e 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1299,6 +1299,16 @@ MapboxGeocoder.prototype = { return this.options.worldview }, + + /** + * Determine whether a marker should be added to the map. + * @returns {Boolean} `true` if a marker should be added, `false` if not + * @private + */ + _shouldAddMarker: function() { + return this.options.marker && this._mapboxgl; + }, + /** * Handle the placement of a result marking the selected result * @private @@ -1328,15 +1338,6 @@ MapboxGeocoder.prototype = { return this; }, - /** - * Determine whether a marker should be added to the map. - * @returns {Boolean} `true` if a marker should be added, `false` if not - * @private - */ - _shouldAddMarker: function() { - return this.options.marker && this._mapboxgl; - }, - /** * Handle the removal of a result marker * @private From 65551aa98dac16ea1454904f793071c855d2d5e1 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:46:40 -0700 Subject: [PATCH 07/13] Use imperative tense in _shouldFly() block comment --- lib/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/index.js b/lib/index.js index 69ea6c8e..c644d2a7 100644 --- a/lib/index.js +++ b/lib/index.js @@ -539,7 +539,7 @@ MapboxGeocoder.prototype = { }, /** - * Determines whether the map should fly to a new location after geocoding. + * Determine whether the map should fly to a new location after geocoding. * @returns {Boolean} `true` if the map should fly to new locations, `false` if not * @private */ From 2d3a241f8523f7dce124dcf753bc81ec01be52bc Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:48:09 -0700 Subject: [PATCH 08/13] Make test more readable --- test/test.geocoder.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index 120c7d9a..c2acfd49 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -830,13 +830,17 @@ test('geocoder', function(tt) { 'result', once(function() { t.ok(flyToSpy.calledOnce, 'flyTo() is called after geolocating'); + const calledWithArgs = flyToSpy.args[0][0]; + t.equals(+calledWithArgs.center[0].toFixed(4), +geolocationPositionStub.coords.longitude.toFixed(4), 'the map is directed to fly to the right longitude'); + t.equals(+calledWithArgs.center[1].toFixed(4), +geolocationPositionStub.coords.latitude.toFixed(4), 'the map is directed to fly to the right latitude'); + t.deepEqual(calledWithArgs.zoom, 16, 'the map is directed to fly to the right zoom level'); }) From d106148ad6be63bd548b558bb36e2dfe13657b1d Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:49:24 -0700 Subject: [PATCH 09/13] Clean up syntax in flyTo() test --- test/test.geocoder.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index c2acfd49..bcf8d0ad 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -738,7 +738,8 @@ test('geocoder', function(tt) { }); tt.test('options.flyTo [false] when querying', function(t){ - t.plan(1) + t.plan(1); + setup({ flyTo: false }); @@ -780,7 +781,8 @@ test('geocoder', function(tt) { }); tt.test('options.flyTo [true] when querying', function(t){ - t.plan(4) + t.plan(4); + setup({ flyTo: true }); From 3f7a62239338b9302e1c1e370913d777d2872678 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 10:50:17 -0700 Subject: [PATCH 10/13] Use consistent quote style in flyTo() test --- test/test.geocoder.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index bcf8d0ad..d641872e 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -787,7 +787,7 @@ test('geocoder', function(tt) { flyTo: true }); - const flyToSpy = sinon.spy(map, "flyTo"); + const flyToSpy = sinon.spy(map, 'flyTo'); geocoder.query('Golden Gate Bridge'); From 6787a05502ad2c9a9c627ba4d593cb8702bc4dec Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 11:20:39 -0700 Subject: [PATCH 11/13] Make options.marker test more readable --- test/test.geocoder.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/test.geocoder.js b/test/test.geocoder.js index d641872e..2642421b 100644 --- a/test/test.geocoder.js +++ b/test/test.geocoder.js @@ -965,11 +965,15 @@ test('geocoder', function(tt) { 'result', once(function(event) { t.ok(markerConstructorSpy.calledOnce, 'a new marker is added to the map'); + const calledWithOptions = markerConstructorSpy.args[0][0]; + t.equals(calledWithOptions.color, '#4668F2', 'a default color is set'); + t.equals(+event.result.user_coordinates[1].toFixed(4), +geolocationPositionStub.coords.latitude.toFixed(4), 'the marker is placed at the correct latitude'); + t.equals(+event.result.user_coordinates[0].toFixed(4), +geolocationPositionStub.coords.longitude.toFixed(4), 'the marker is placed at the correct longitude'); From 35f947adf907a5b0ff87e2cefcff846c7915b848 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 11:35:55 -0700 Subject: [PATCH 12/13] Remove rogue blank line --- lib/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/index.js b/lib/index.js index c644d2a7..7583afec 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1299,7 +1299,6 @@ MapboxGeocoder.prototype = { return this.options.worldview }, - /** * Determine whether a marker should be added to the map. * @returns {Boolean} `true` if a marker should be added, `false` if not From 0c13a7f05b23f0306086a93e8718260d03647de2 Mon Sep 17 00:00:00 2001 From: Ross Paffett Date: Mon, 20 Jun 2022 11:38:55 -0700 Subject: [PATCH 13/13] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 50bbbbd9..21962e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ ## HEAD +### Bug fixes 🐛 + +- The `marker` and `flyTo` options are now respected when geolocation is enabled [#462](https://github.com/mapbox/mapbox-gl-geocoder/pull/462) + ## 5.0.1 ### Bug fixes 🐛