No Map #295

scottsfarley93 · 2019-09-19T17:42:46Z

This PR allows using the mapbox-gl-geocoder plugin without a map, as requested in #270.

Currently, the plugin is added to the map using the map's addControl method. In order to facilitate adding the map to other elements, I added a new addTo method on the geocoder class. If the addTo method is called with a mapbox-gl-js map, it will be added like normal (the map's addControl method will be called under the hood). If, on the other hand, geocoder.addTo is called with a string that looks like a reference to an HTML class or ID, it will add the search bar to that element. In these cases, the geocoder will have no _map property and all map-related methods (like updateProximity or flyTo result) will not be executed.

This still needs tests and I want to make sure there's no edge cases where the lack of a map causes unexpected problems.

I will also need to:

return the debug page to it's original state
Add a new mapbox-gl-js example that shows how to use the geocoder without the map

\cc @kbauhausness @shabnomnom

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
run npm run docs and commit changes to API.md
update CHANGELOG.md with changes under master heading before merging

debug/index.js

scottsfarley93 · 2019-09-24T22:28:27Z

Tapping @yuletide for review.

andrewharvey · 2019-09-25T07:50:02Z

Is it worth linking to the Mapbox TOS obligations when using places? https://www.mapbox.com/legal/tos#[GAGA], since you wouldn't be able to use no map unless you exclude places.

yuletide

This is awesome. Added a few comments and suggestions

yuletide · 2019-09-27T22:50:07Z

README.md

@@ -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. 


README.md

yuletide · 2019-09-27T22:59:28Z

lib/events.js

    //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)


Suggested change

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

yuletide · 2019-09-27T23:00:16Z

lib/events.js

    //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)
+    console.log(payload)


Suggested change

console.log(payload)

yuletide · 2019-09-27T23:16:00Z

lib/index.js

+        throw new Error("Element ", container, "not found.")
+      }
+
+      parent.forEach(function(parentEl){


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

lib/index.js

yuletide · 2019-09-27T23:26:30Z

lib/index.js

@@ -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);


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

yuletide · 2019-09-27T23:44:30Z

debug/nomap.js

+notMapDiv.style.top = 0;
+notMapDiv.style.right = 0;
+notMapDiv.style.left = 0;
+notMapDiv.style.bottom = 0;


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');

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

yuletide · 2019-09-27T23:46:55Z

lib/index.js

+   * 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){


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)

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.

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/

yuletide · 2019-09-27T23:49:19Z

lib/index.js

+      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('.'))){


Suggestion: Allow passing in an HTMLElement

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

Co-Authored-By: Alex Yule <adyule@gmail.com>

pauloanalista · 2019-10-22T16:40:39Z

Does anyone know if this issue has been resolved?

I want to replace Google Maps with MapBox and use a places autocomplete and I don't have a map.

scottsfarley93 · 2019-10-22T17:01:54Z

@pauloanalista this feature was released in v4.5.0. See some documentation here.

start work on geocoder without map

c15ddb1

scottsfarley93 mentioned this pull request Sep 19, 2019

Allow use of geocoder without map #270

Closed

andrewharvey reviewed Sep 19, 2019

View reviewed changes

debug/index.js Outdated Show resolved Hide resolved

Scott Farley added 7 commits September 24, 2019 14:36

tests for geocoder without a map

7f53ecf

changelog

8173efd

new API docs for geocoder.addTo function

a6d0e70

new debug page for working without a map

5830a8b

return main debug page to original state

1a33613

don't send zoom in usage event if the plugin has no map

533e088

fix syntax to work with build script

7ffa0ee

scottsfarley93 changed the title ~~[WIP] No Map~~ No Map Sep 24, 2019

scottsfarley93 requested review from yuletide and andrewharvey September 24, 2019 22:28

bind to this

0eb94ba

add link to TOS in readme

dc7ab50

yuletide suggested changes Sep 27, 2019

View reviewed changes

Scott Farley and others added 4 commits September 30, 2019 10:54

only add the geocoder to a single element

12adb63

Update README.md according to @yuletide suggestion

e637f10

Co-Authored-By: Alex Yule <adyule@gmail.com>

disable eslint for missing mapboxgl error

644ccfd

Co-Authored-By: Alex Yule <adyule@gmail.com>

move if map statement for clarity

bb5304b

scottsfarley93 mentioned this pull request Oct 2, 2019

HTML Debug Page #299

Open

4 tasks

merge upstream master, resolve changelog conflicts

bc89d5e

scottsfarley93 merged commit ccded5b into master Oct 10, 2019

scottsfarley93 deleted the sans-map branch October 10, 2019 18:33

andrewharvey mentioned this pull request Jul 12, 2020

fix failing test which was missing bind, add tests for element scenario #382

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No Map #295

No Map #295

scottsfarley93 commented Sep 19, 2019 •

edited

Loading

scottsfarley93 commented Sep 24, 2019

andrewharvey commented Sep 25, 2019

yuletide left a comment

yuletide Sep 27, 2019

yuletide Sep 27, 2019

yuletide Sep 27, 2019

yuletide Sep 27, 2019

yuletide Sep 27, 2019

yuletide Sep 27, 2019

scottsfarley93 Sep 30, 2019

yuletide Sep 27, 2019

scottsfarley93 Sep 30, 2019

yuletide Oct 7, 2019 •

edited

Loading

yuletide Sep 27, 2019

pauloanalista commented Oct 22, 2019

scottsfarley93 commented Oct 22, 2019

-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');

No Map #295

No Map #295

Conversation

scottsfarley93 commented Sep 19, 2019 • edited Loading

Launch Checklist

scottsfarley93 commented Sep 24, 2019

andrewharvey commented Sep 25, 2019

yuletide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuletide Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pauloanalista commented Oct 22, 2019

scottsfarley93 commented Oct 22, 2019

scottsfarley93 commented Sep 19, 2019 •

edited

Loading

yuletide Oct 7, 2019 •

edited

Loading