Skip to content

add extra mapbox-gl-geocoder examples - #5915

Merged
andrewharvey merged 3 commits into
mb-pagesfrom
geocoder-examples
Jan 22, 2018
Merged

andrewharvey merged 3 commits into
mb-pagesfrom
geocoder-examples

Conversation

@andrewharvey

@andrewharvey andrewharvey commented Dec 23, 2017

Copy link
Copy Markdown
Collaborator

Launch Checklist

  • briefly describe the changes in this PR

This tackles the first part of mapbox/mapbox-gl-geocoder#138 adding more examples of mapbox-gl-geocoder. I think it's better to keep all GL JS ecosystem examples together here making them easier to find and to re-use existing infrastructure compared to having a separate mapbox-gl-geocoder examples page.

Additional examples are:

Before this can be merged:

  • gather feedback / review
  • coordinate a new release of mapbox-gl-geocoder
  • update version number of mapbox-gl-geocoder contained within this PR

@andrewharvey
andrewharvey changed the base branch from master to gh-pages December 24, 2017 06:20
@andrewharvey
andrewharvey changed the base branch from gh-pages to mb-pages January 3, 2018 08:23
@andrewharvey andrewharvey mentioned this pull request Jan 18, 2018
8 tasks
@andrewharvey
andrewharvey requested review from tristen and removed request for tristen January 18, 2018 10:44
@andrewharvey andrewharvey changed the title [wip] add extra mapbox-gl-geocoder examples add extra mapbox-gl-geocoder examples Jan 21, 2018
@andrewharvey

Copy link
Copy Markdown
Collaborator Author

This PR is ready now.

@tristen tristen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Examples look great!

filter: function (item) {
// returns true if item contains New South Wales region
return item.context.map((i) => {
return (i.id.startsWith('region') && i.text === "New South Wales");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's no support in IE for startsWith right?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

That's right.

I've included a polyfill since I feel startsWith is much more readable than the alternative of i.id.substr(0, 6) === 'region' and simpler than a regexp.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 looks like that just needs a no-extend-native line for tests to pass.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, but after thinking some more I think it's probably better to simply split and shift since "id of the feature of the form {index}.{id}"

description: >-
Use the [mapbox-gl-geocoder](https://github.com/mapbox/mapbox-gl-geocoder)
control to search for places using Mapbox Geocoding API limiting results
to a region

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Period

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

type: "Point",
coordinates: [lng, lat]
},
place_name: 'Lat: ' + lat + ', Lng: ' + lng, // eslint-disable-line camelcase

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm that's annoying we have to eslint disable here but 🤷‍♂️

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes but I don't see any other way around it since place_name is defined in the carmen geojson spec

Use the [mapbox-gl-geocoder](https://github.com/mapbox/mapbox-gl-geocoder)
control to search for places using Mapbox Geocoding API, supplementing
results from a local data source or function, in this case to match
geographic coordinates provided as input

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Period

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

description: >-
Use the [mapbox-gl-geocoder](https://github.com/mapbox/mapbox-gl-geocoder)
control to search for places using Mapbox Geocoding API on an input search
box external to the map

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Period

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,13 @@
/*---
title: Use the geocoder on an external search box

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could the title and description be worded a little differently? I'm not sure a user looking to add a geocoder to an element outside the map would think to search "external search box".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I've changed the title to: "Place the geocoder input outside the map" and descirption to:

Use the mapbox-gl-geocoder
control to search for places using Mapbox Geocoding API attached to an
element outside the map

@andrewharvey

andrewharvey commented Jan 21, 2018

Copy link
Copy Markdown
Collaborator Author

Thanks for the review @tristen, I've implemented some changes.

@andrewharvey
andrewharvey merged commit 9c601e1 into mb-pages Jan 22, 2018
@andrewharvey
andrewharvey deleted the geocoder-examples branch January 22, 2018 09:53
@ChrisLoer

Copy link
Copy Markdown
Contributor

@andrewharvey Not sure if it's worth ticketing, but I just ran across mapbox-gl-geocoder-local-geocoder while doing manual testing, and it was pretty hard to figure out what it was supposed to do. In fact, even looking at that regex, I still haven't managed to enter a search query that returns results that seem to be based on lat/long. Maybe it would help to add a line to the description providing an example query using the functionality?

@andrewharvey

Copy link
Copy Markdown
Collaborator Author

hey @ChrisLoer it's supposed to allow the implementer to do mapbox/mapbox-gl-geocoder#121, but being generic so you can supplement the geocoder with your own non-mapbox geocoder data, eg from your Dataset

is this the unclear regex?

// match anything which looks like a decimal degrees coordinate pair
var matches = query.match(/^[ ](-?\d+.?\d)[, ]+(-?\d+.?\d*)[ ]*$/);

to match queries like

-40.1, 160.9
-40.1 160.9
160.9,-40.1

as their coordinates

@ChrisLoer

Copy link
Copy Markdown
Contributor

🤔 I thought I tried the simple "x,y" syntax and I was getting results with random addresses that contained those numbers. But I can't reproduce that behavior now -- works easily.

Probably just ignore me. 😳 Although adding something like "provided as input in the format ', '" would still make the example a bit more accessible I think.

@mollymerp

Copy link
Copy Markdown
Contributor

hmm we're getting to the point where a Geocoder subheading in the examples nav might be worthwhile 💭

thanks @andrewharvey !

@andrewharvey andrewharvey mentioned this pull request Apr 4, 2018
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants