allow for multiple osm_tags #21
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
May 30, 2018 19:21
verifies that non-strings have a length > 0. removes string encoding - osm_tag place:value combos are currently broken due to encoding the colon.
Contributor
|
Thanks for this work!
|
| }, | ||
|
|
||
| _onSelected: function (feature) { | ||
| this.map.setView([feature.geometry.coordinates[1], feature.geometry.coordinates[0]], 16); |
Contributor
There was a problem hiding this comment.
This change seems unrelated to the current topic.
| if (params[key]) { | ||
| queryString.push(encodeURIComponent(key) + '=' + encodeURIComponent(params[key])); | ||
| if (params[key] && (typeof(params[key]) == 'string' || typeof(params[key]) == 'number')) { | ||
| queryString.push(encodeURIComponent(key) + '=' + params[key]); |
Contributor
There was a problem hiding this comment.
This line should be factorized with the line 38.
Basically I think the pattern is to always process an array, and turn the param in an array when it's not.
Author
|
uh... yah, unrelated and unintended. I couldn't figure out how to
override this behavior - please ignore.
…On Wed, 06 Jun 2018 11:15:59 -0700 Yohan Boniface ***@***.***> wrote:
yohanboniface commented on this pull request.
> @@ -372,7 +376,7 @@ L.PhotonSearch = L.PhotonBaseSearch.extend({
},
_onSelected: function (feature) {
- this.map.setView([feature.geometry.coordinates[1],
feature.geometry.coordinates[0]], 16);
This change seems unrelated to the current topic.
|
Author
|
You are correct, however, photon is not processing encoded colons
properly. I suppose the real fix is to kick it over to photon as a
bug... as I was under a time crunch I treated it as a feature. =)
j
…On Wed, 06 Jun 2018 18:15:35 +0000 (UTC) Yohan Boniface ***@***.***> wrote:
Thanks for this work!
The array support seems legit, but I've some comments.
> calling encodeURIComponent on osm_tag(s) breaks place:city
`:` is not valid as query string key, afaik:
https://tools.ietf.org/html/rfc3986#section-2.2
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
update buildQueryString() to accept array values. This allows for passing an array of osm_tag values and having them all passed to the photon server correctly.
calling encodeURIComponent on osm_tag(s) breaks place:city