Skip to content
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

No Map #295

Merged
merged 15 commits into from
Oct 10, 2019
380 changes: 200 additions & 180 deletions API.md

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## Master
### Features / Improvements 🚀

- Supports adding a geocoder to an arbitrary HTML element so it can be used without a map [#270](https://github.com/mapbox/mapbox-gl-geocoder/issues/270).


## v4.4.2

### Features / Improvements 🚀
Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ so in browserify or webpack, you can require it like:
var MapboxGeocoder = require('@mapbox/mapbox-gl-geocoder');
```

### Using without a Map
It is possible to use the plugin without it being placed as a control on a mapbox-gl map. Keep in mind that the Mapbox [Terms of Service](https://www.mapbox.com/legal/tos#[GAGA]) require that POI search results be shown on a Mapbox map. If you don't need a POIs, you can exclude them from your search results with the `options.types` parameter when constructing a new Geocoder.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor typo

scottsfarley93 marked this conversation as resolved.
Show resolved Hide resolved

### Deeper dive

#### API Documentation
Expand Down
4 changes: 3 additions & 1 deletion debug/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ var geocoder = new MapboxGeocoder({
mapboxgl: mapboxgl
});

map.addControl(geocoder)

window.geocoder = geocoder;

var button = document.createElement('button');
Expand All @@ -96,7 +98,6 @@ map
.getContainer()
.querySelector('.mapboxgl-ctrl-bottom-left')
.appendChild(button);
map.addControl(geocoder);

map.on('load', function() {
button.addEventListener('click', function() {
Expand All @@ -116,6 +117,7 @@ geocoder.on('result', function(e) {
});

geocoder.on('clear', function(e) {
console.log(e)
console.log('clear');
});

Expand Down
60 changes: 60 additions & 0 deletions debug/nomap.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'use strict';
var mapboxgl = require('mapbox-gl');
var insertCss = require('insert-css');
var fs = require('fs');

mapboxgl.accessToken = window.localStorage.getItem('MapboxAccessToken');


var meta = document.createElement('meta');
meta.name = 'viewport';
meta.content = 'initial-scale=1,maximum-scale=1,user-scalable=no';
document.getElementsByTagName('head')[0].appendChild(meta);

insertCss(fs.readFileSync('./lib/mapbox-gl-geocoder.css', 'utf8'));
insertCss(
fs.readFileSync('./node_modules/mapbox-gl/dist/mapbox-gl.css', 'utf8')
);

var MapboxGeocoder = require('../');

var notMapDiv = document.body.appendChild(document.createElement('div'));
notMapDiv.style.position = 'absolute';
notMapDiv.style.top = 0;
notMapDiv.style.right = 0;
notMapDiv.style.left = 0;
notMapDiv.style.bottom = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reproduce the double stuff add in:

Suggested change
notMapDiv.style.bottom = 0;
notMapDiv.style.bottom = '50%';
var anotherNotMapDiv = document.body.appendChild(document.createElement('div'));
anotherNotMapDiv.style.position = 'absolute';
anotherNotMapDiv.style.top = '50%';
anotherNotMapDiv.style.right = 0;
anotherNotMapDiv.style.left = 0;
anotherNotMapDiv.style.bottom = 0;
anotherNotMapDiv.style.backgroundColor = 'green';
anotherNotMapDiv.classList.add('notAMap');

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test to test.ui.js, so I'm not sure we need to add this to the debug page.

notMapDiv.style.backgroundColor = 'darkcyan';

notMapDiv.classList.add("notAMap");

var geocoder = new MapboxGeocoder({
accessToken: mapboxgl.accessToken,
trackProximity: true
});

geocoder.addTo('.notAMap')

window.geocoder = geocoder;


geocoder.on('results', function(e) {
console.log('results: ', e.features);
});

geocoder.on('result', function(e) {
console.log('result: ', e.result);
});

geocoder.on('clear', function(e) {
console.log(e)
console.log('clear');
});

geocoder.on('loading', function(e) {
console.log('loading:', e.query);
});

geocoder.on('error', function(e) {
console.log('Error is', e.error);
});
5 changes: 4 additions & 1 deletion lib/events.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,16 @@ MapboxEventManager.prototype = {
*
*/
keyevent: function(keyEvent, geocoder){

//pass invalid event
if (!keyEvent.key) return;
// don't send events for keys that don't change the input
// TAB, ESC, LEFT, RIGHT, ENTER, UP, DOWN
if (keyEvent.metaKey || [9, 27, 37, 39, 13, 38, 40].indexOf(keyEvent.keyCode) !== -1) return;
var payload = this.getEventPayload('search.keystroke', geocoder);
payload.lastAction = keyEvent.key;
console.log("action: ", payload.lastAction)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log("action: ", payload.lastAction)

console.log(payload)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(payload)

if (!payload.queryString) return; // will be rejected
return this.push(payload);
},
Expand Down Expand Up @@ -152,7 +155,7 @@ MapboxEventManager.prototype = {
if (!geocoder.options.proximity) proximity = null;
else proximity = [geocoder.options.proximity.longitude, geocoder.options.proximity.latitude];

var zoom = (geocoder._map) ? geocoder._map.getZoom() : null;
var zoom = (geocoder._map) ? geocoder._map.getZoom() : undefined;
var payload = {
event: event,
created: +new Date(),
Expand Down
71 changes: 59 additions & 12 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,43 @@ MapboxGeocoder.prototype = {
}
},

/**
* Add the geocoder to a container. The container can be either a mapboxgl.Map or a reference to an HTML class or ID.
*
* If the container is a mapboxgl.Map, this function will behave identically to Map.addControl(geocoder)
* If the container is an html id or class, the geocoder will be appended to that element
*
* This function will throw an error if the container is not one a map or a class/id reference
* It will also throw an error if the referenced html element cannot be found in the document.body
* @param {String|mapboxgl.Map} container A reference to the container to which to add the geocoder
*/
addTo: function(container){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the new recommended way to invoke the geocoder? If so we should update the debug/index.js and docs to recommend using this method first, and not onAdd (which I think was always awkward so this is a welcome change)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the recommended method should still be map.addControl(geocoder) -- since I believe this will still be the primary usage. We can add an example that uses this method though.

Copy link
Contributor

@yuletide yuletide Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. So map.addControl if you have a map, otherwise use addTo(container)

This is the example I had in mind that we could update to use addTo instead of onAdd (which sounds like and should probably be a private, internal method) since its more logical: https://docs.mapbox.com/mapbox-gl-js/example/mapbox-gl-geocoder-outside-the-map/

// if the container is a map, add the control like normal
if (container._controlContainer){
// it's a mapbox-gl map, add like normal
container.addControl(this)
}
// if the container is not a map, but an html element, then add the control to that element
else if (typeof container == 'string' && (container.startsWith('#') || container.startsWith('.'))){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Allow passing in an HTMLElement

if (container instanceof HTMLElement){parent = container} and skip the querySelector bit

const parent = document.querySelectorAll(container);
if (parent.length == 0){
throw new Error("Element ", container, "not found.")
}

parent.forEach(function(parentEl){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to create two geocodes with the same class applied to two different elements, but they behave unpredictably once created
gif

const el = this.onAdd(); //no map
parentEl.appendChild(el);
}.bind(this))

}else{
throw new Error("Error: addTo Container must be a mapbox-gl-js map or a html element reference")
}
},

onAdd: function(map) {
this._map = map;
if (map && typeof map != 'string'){
this._map = map;
}

this.setLanguage();

Expand Down Expand Up @@ -182,20 +217,20 @@ MapboxGeocoder.prototype = {
this.setRenderFunction(this.options.render);
this._typeahead.getItemValue = this.options.getItemValue;

if (this.options.trackProximity) {
if (this.options.trackProximity && this._map) {
this._updateProximity();
this._map.on('moveend', this._updateProximity);
}

this.mapMarker = null;
this._handleMarker = this._handleMarker.bind(this);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would never be called if there's no map right? I'd move this stuff to the if (this._map) block below for simplicity. Also means you don't need to check for map in the handler


this._mapboxgl = this.options.mapboxgl;
if (!this._mapboxgl && this.options.marker) {
console.error("No mapboxgl detected in options. Map markers are disabled. Please set options.mapboxgl.");
this.options.marker = false;
if (this._map){
this._mapboxgl = this.options.mapboxgl;
if (!this._mapboxgl && this.options.marker) {
scottsfarley93 marked this conversation as resolved.
Show resolved Hide resolved
console.error("No mapboxgl detected in options. Map markers are disabled. Please set options.mapboxgl.");
this.options.marker = false;
}
}

return el;
},

Expand All @@ -213,7 +248,7 @@ MapboxGeocoder.prototype = {
onRemove: function() {
this.container.parentNode.removeChild(this.container);

if (this.options.trackProximity) {
if (this.options.trackProximity && this._map) {
this._map.off('moveend', this._updateProximity);
}

Expand Down Expand Up @@ -279,7 +314,9 @@ MapboxGeocoder.prototype = {
if (selected.properties && !exceptions[selected.properties.short_code] && selected.bbox) {
var bbox = selected.bbox;
flyOptions = extend({}, this.options.flyTo);
this._map.fitBounds([[bbox[0], bbox[1]], [bbox[2], bbox[3]]], flyOptions);
if (this._map){
this._map.fitBounds([[bbox[0], bbox[1]], [bbox[2], bbox[3]]], flyOptions);
}
} else if (selected.properties && exceptions[selected.properties.short_code]) {
// Certain geocoder search results return (and therefore zoom to fit)
// an unexpectedly large bounding box: for example, both Russia and the
Expand All @@ -288,15 +325,19 @@ MapboxGeocoder.prototype = {
// at ./exceptions.json provides "reasonable" bounding boxes as a
// short-term solution; this may be amended as necessary.
flyOptions = extend({}, this.options.flyTo);
this._map.fitBounds(exceptions[selected.properties.short_code].bbox, flyOptions);
if (this._map){
this._map.fitBounds(exceptions[selected.properties.short_code].bbox, flyOptions);
}
} else {
var defaultFlyOptions = {
zoom: this.options.zoom
}
flyOptions = extend({}, defaultFlyOptions, this.options.flyTo);
// ensure that center is not overriden by custom options
flyOptions.center = selected.center;
this._map.flyTo(flyOptions);
if (this._map){
this._map.flyTo(flyOptions);
}
}
}
if (this.options.marker && this._mapboxgl){
Expand Down Expand Up @@ -513,6 +554,9 @@ MapboxGeocoder.prototype = {
_updateProximity: function() {
// proximity is designed for local scale, if the user is looking at the whole world,
// it doesn't make sense to factor in the arbitrary centre of the map
if (!this._map){
return;
}
if (this._map.getZoom() > 9) {
var center = this._map.getCenter().wrap();
this.setProximity({ longitude: center.lng, latitude: center.lat });
Expand Down Expand Up @@ -848,6 +892,9 @@ MapboxGeocoder.prototype = {
*/
_handleMarker: function(selected){
// clean up any old marker that might be present
if (!this._map){
return;
}
this._removeMarker();
var defaultMarkerOptions = {
color: '#4668F2'
Expand Down
Loading