Reversegeocode #5976
Reversegeocode #5976
Conversation
…eocoder.html, to do reverse geocoding using the default geocoder, Bing Maps
…n the Reverse Geocoder
|
Welcome to the Cesium community @marhab21! Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
I have already sent a CLA. I failed eslint, but corrected the errors. |
|
What do I do to get the code reviewed? |
|
Still says CLA Required...hmmm...I sent it to cla@agi.com...should I do something else? |
|
Hi @marhab21, we're a bit busy this week, but someone should have time to review your pull request soon. Thanks! |
|
Hi Hannah!
Thank you, no problem, I was just wondering if there was anything else I
needed to do to get a review...
No hurry :)
Thanks again,
Martine
…On Mon, Nov 13, 2017 at 5:12 PM, Hannah ***@***.***> wrote:
Hi @marhab21 <https://github.com/marhab21>, we're a bit busy this week,
but someone should have time to review your pull request soon. Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5976 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ATJUaAH3hJbGwC8nig2VmYZELi_2NOzDks5s2OkBgaJpZM4QcRhS>
.
|
|
We now have a CLA from @marhab21, thanks again! |
.gitignore
Outdated
| @@ -10,7 +10,7 @@ Thumbs.db | |||
| /Apps/CesiumViewer/Gallery/gallery-index.js | |||
|
|
|||
| /Apps/Sandcastle/jsHintOptions.js | |||
| /Apps/Sandcastle/gallery/gallery-index.js | |||
| #/Apps/Sandcastle/gallery/gallery-index.js | |||
There was a problem hiding this comment.
Can you revert this change?
There was a problem hiding this comment.
We don't want to commit gallery-index.js. It is generated automatically when you run npm run build
| * @alias OpenStreetMapNominatimGeocoder | ||
| * @constructor | ||
| */ | ||
| * This class is an example of a custom geocoder, and reverse geocoder. It provides both * * through the OpenStreetMap Nominatim service. Press the "Reverse Geocoder" button to go * * to OpenStreet Map headquarters. OpenStreetMap has more information in the UK than in the * US. |
There was a problem hiding this comment.
No, I think it is a result of formatting when I got rid of the tabs...will fix : )
| /** | ||
| * Find the camera position after search, and put a point there. | ||
| */ | ||
| function setPointForSearchLocation() { |
There was a problem hiding this comment.
Something in this function is causing an error to be thrown when you use the geocoder search
|
Thanks for the pull request @marhab21! I think these demos will be helpful for people. |
|
Yes, I will...sorry...thanks Hannah!
Martine
…On Tue, Nov 28, 2017 at 12:04 PM, Hannah ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In .gitignore
<#5976 (comment)>
:
> @@ -10,7 +10,7 @@ Thumbs.db
/Apps/CesiumViewer/Gallery/gallery-index.js
/Apps/Sandcastle/jsHintOptions.js
-/Apps/Sandcastle/gallery/gallery-index.js
+#/Apps/Sandcastle/gallery/gallery-index.js
Can you revert this change?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5976 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ATJUaOKSv-PZiDy0TP8MmZIqS5RfMJViks5s7Gc4gaJpZM4QcRhS>
.
|
… but somehow not checked in :(
|
OK, I hope I have fixed everything. Do I need to close the pull request, or do I wait 'til it's merged? |
|
You can just leave this PR open. I'll take another look at it tomorrow. Does anyone else want to look at these sandcastle examples? @pjcozzi? |
|
Sounds like a great example, but I have to pass due to time constraints. |
|
While I'm fine with having a reverse geocoding example (though I haven't run it yet), I'm not sure updating the custom geocoder example to add reverse geocoding is a good idea, I think it confuses what is otherwise a solid simple example. Since this is all just example code (no Cesium to changes at all) I think just adding the new example would be preferred. |
|
Hi Matthew!
Thank you...
Should I revert the change on the reverse part of the Custom Geocoder?
Let me know if there is anything I need to do!
Thanks again,
Martine
…On Tue, Nov 28, 2017 at 7:08 PM, Matthew Amato ***@***.***> wrote:
While I'm fine with having a reverse geocoding example (though I haven't
run it yet), I'm not sure updating the custom geocoder example to add
reverse geocoding is a good idea, I think it confuses what is otherwise a
solid simple example. Since this is all just example code (no Cesium to
changes at all) I think just adding the new example would be preferred.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5976 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ATJUaDrf4fKdx7erQ1V7u7N6iohwGXgYks5s7MqvgaJpZM4QcRhS>
.
|
|
@marhab21 Revert the changes in the Custom Geocoder example, and add the code to a new Sandcastle example. You should make a new file like |
|
@ggetz there is already a new Sandcastle example, we just need to revert the custom geocoder example and that's it. There's no reason to have two reverse geocoding examples. If you could reset this branch to clean up the history, that would be great too; but if you're not sure how to do that, don't worry about it. |
|
Thank you! I will revert the Custom Geocoder...I am not sure about how to reset the branch. I will look into it, but might leave it alone...I will confirm one way or the other. |
|
Well, I have tried everything, but my builds keep failing, and I can't figure out why. I did catch 1 lint error, which I fixed. But the build keeps failing mainly in KML files, and things that don't seem to have anything to do with my changes. What am I missing? Here is the raw log... |
|
No worries @marhab21. We've had some problems with CI randomly failing lately. I'll take a closer look when I have a minute to do another review here, but It doesn't seem related to your changes. |
|
Thank you Hannah!! : ) |
|
This is a super cool demo, thanks for the contribution @marhab21! I just pushed a commit with some code cleanup so your example follows the same coding guidelines as our other demos. I also removed the code to put a point at the location when you enter an address into the geocoder search bar because it wasn't as relevant to this example. Congratulations on your first contribution! |
|
Just noticed that the formatting for |
|
Thank you everyone! I am so happy to have a contribution in Cesium!! And thanks for making the last minute fixes... |
|
I found it in "Showcases", but it doesn't seem to load, and redirects to HelloWorld :( |
It will go out on Friday with the next Cesium release (Cesium releases on the first business day of every month). |
|
Awesome! Thank you so much!!
Martine
…Sent from my iPhone
On Nov 29, 2017, at 4:25 PM, Matthew Amato ***@***.***> wrote:
When will the demo be up in Official Sandcastle?
It will go out on Friday with the next Cesium release (Cesium releases on the first business day of every month).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I added a Reverse Geocoder to the Custom Geocoder.html Sandcastle demo, and I also added a new demo, which is another Reverse Geocoder, using BingMaps, and giving the address every time the user clicks on a location on the map.
This will demonstrate various features of Cesium, including comparing the information retrieved from OpenStreetMap to the one retrieved from Bing Maps.
Let me know what documentation or what else I need to add, or what I might be doing wrong : )
I had to edit .gitignore in order to add a gallery item.