Geocoding widget #1157

kring · 2013-09-18T21:30:46Z

There's a lot more I could do here, but I think it's reasonable to merge it into master in its current state.

I suspect the most controversial thing here is the location of the new widget in the Viewer. I originally had it in the top left, but @emackey made the excellent point that we should reserve that for app-specific functionality.

This branch includes all the bingKey changes as well, so the diff will get simpler once that branch is merged.

Conflicts: Source/Core/BingMapsApi.js

mramato · 2013-09-18T23:23:17Z

I'll get this party started. I absolutely hate the location in the upper right. I have two thoughts:

In the Cesium Viewer application, it should be in the upper left. It's an app so there's no reason to reserve space for others. Eventually we'll have to incorporate it with the data source browser somehow.
In the Viewer widget, I see two options.
- Don't include it at all, in my opinion it's kind of outside the scope of what the Viewer widget is for (a base to build applications. What are the chances that someone wants to have a custom UI in their application and at the same time keep our stock geocoding widget behavior?
- Include it in the widget in the upper left and people can easily use CSS to move it if they want to use the upper left or they can simply pass in false to the constructor to make it go away.

The upper left is the natural best fit and that's where it should go.

Now with that out of the way, I'll spend some time this week looking at the code and playing with the widget.

kring · 2013-09-19T01:45:37Z

Ha, well, give it some time. It grew on me quite a bit.

I think we're really used to something like that being in the upper left. But that's because we're thinking of apps like Google Maps, where that widget practically is the application. It makes sense for it to be in the most prominent location - the upper left. In Cesium, geocoding is useful, but it's not what we're all about. It makes sense to me to de-emphasize it by moving it elsewhere. And it's pragmatic, too.

At one point I had it to the left of the buttons, instead of above them. I liked that better, but it didn't work well on mobile, at least in portrait mode. I could see putting it back there and making it narrower, and then using some CSS magic to make it wider when it's selected.

I definitely think this belongs in the Viewer widget though. Sure, it may not be useful in every app - that's why you can turn off all the widgets in the viewer, including this one - but it's certainly at least as likely to be useful as the imagery layer picker. Also, it's my understanding that the Viewer widget is supposed to pretty much be our Cesium Viewer app, in widget form, so it doesn't make sense to me to have it on by default in Cesium Viewer, but not in the Viewer widget.

Incorporating it with the data source browser in the future makes sense to me.

Another idea is to demote it to just a button in the Viewer widget. Hit the button and the input box appears below the row of buttons. I'm not sure if I like that better, but it's in line with my theory that geocoding should be de-emphasized in Cesium relative to other mapping apps.

pjcozzi · 2013-09-19T12:38:23Z

This shouldn't hold up this pull request, but flying from the north pole to the south pole creates a strange flight. I know, I test the craziest things. CC @bagnell

pjcozzi · 2013-09-19T12:53:29Z

Not really my area but to help move us along...

I don't like that the Chrome FPS covers it up in the upper-right, but that is not our problem. The FPS always covers things up.

I do like that it is near the home button, which is pretty similar functionality. As @kring suggested, I suspect it would look better as a single line across the top when screen real estate permits - most sane cases I imagine. If it's a reasonable amount of work, this would be worth seeing.

Again, not my area, but I would expect this to be in both the viewer widget and the viewer app. I thought the app was just an instance of the widget. If an app doesn't need it, they just remove it like we did for the other buttons for the Orbital app (I suppose; @mramato wrote the code).

kring · 2013-09-19T13:08:29Z

Here's what it looks like next to the buttons at the top, and with a smaller input box:

If anyone wants to play with this live I can check it in. Just a tiny tweak to the CSS to change between this and the original layout.

mramato · 2013-10-23T00:08:10Z

I'm going to make a few changes to this, I can open a PR if you want, but it's basically just making the focus behavior work like our other widgets.

Fix CHANGES Move `padding-left` to wide and hover classes so that collapsed version is the right size Add `hasFocus` binding and document listeners so that the text box disappears in all expected situations. Cleanup knockout property declarations.

Using the `hasFocus` binding with a knockout-es5 property seems to cause issues. Instead, bind directly to a low-level observable.

mramato · 2013-10-23T01:47:14Z

Okay, I fixed a bunch of stuff; give it a try and let me know what you think.

@shunter For some reason the GeocoderViewModel spec fails when run under coverage but passes normally. Any ideas?

mramato · 2013-10-23T01:55:45Z

@shunter never mind, it's working now.

mramato · 2013-10-23T02:33:36Z

I think this is just about ready but...

There's a minor knockout issue with the hasFocus binding and es5 knockout properties, but I need to look more into it. It might be a knockout bug. fixed by not using knockout for this part now
On mobile, touches do not affect focus like I would expect, but this is minor for now. This is a hover issue, I'm not sure there's anything we can do about it for now
One thing that would be nice to have is a single button to clear the text field if there's any content in it, but I guess that can wait until later.
Another random thought, should hitting the home button clear the geocoder?

For some reason this still doesn't work as expected on mobile. I think our touch handling in the cameara may be eating the event.

mramato · 2013-10-23T13:48:39Z

It just occurred to me that none of our other widgets active on hover (SceneModePicker, BaseLayerPicker) should be be consistent? The main problem is that hover gives us issues on mobile (to the point where I would recommend we get rid of hover completely for Geocoder and require a click). Thoughts?

emackey · 2013-10-23T13:51:07Z

Text-entry widgets are traditionally a little different than drop-down widgets. I think the current behavior is good.

mramato · 2013-10-23T14:01:10Z

My only real issue with the current behavior is the mobile issue. I think we can work around this by removing the hover affect for touch-enabled devices, but it's not critical for this PR.

mramato · 2013-10-23T14:06:21Z

Okay, I'm done tweaking this unless anyone has anymore feedback. @emackey, let us know when you are done as well and then @shunter can do the final review and merge.

mramato · 2013-10-23T14:11:36Z

Another random thought, we need to expose an even for detecting when the generate flight is complete, this way a user can perform some action (such as setting the camera transform so rotation is around the geolocated position). This can be in a future PR, I just wanted to mention it here. Once this is in master, I'll write up separate Github issues for any ideas in this PR that didn't make it in.

emackey · 2013-10-23T14:31:55Z

Source/Widgets/Geocoder/GeocoderViewModel.js

+         * @type {Boolean}
+         * @default false
+         */
+        knockout.defineProperty(this, 'isSearchInProgress', {


Sadly, jsDoc doesn't pick this up, it's not smart enough to parse knockout.defineProperty.

Same comment for searchText, flightDuration, etc.

Good catch, I'll fix it.

emackey · 2013-10-23T14:46:35Z

This branch looks good. 👍

@shunter can you review & merge? Thanks!

shunter · 2013-10-23T14:57:02Z

It looks like somewhere along the line, the magnifying glass icon lost its click handler, so clicking it does nothing. I assume this was a mistake?

mramato · 2013-10-23T15:03:02Z

I'm looking at it now.

mramato · 2013-10-23T15:10:27Z

I'm not sure how it ever worked, should be fixed now.

emackey · 2013-10-23T15:21:52Z

FWIW, it worked by submitting the form. Maybe the button should be a real button not a span, and set .type='submit' ? Or just merge this 😄

Geocoding widget

mramato · 2013-10-23T15:38:08Z

Now none of us have to get @pjcozzi anything for Christmas 😄

kring added 24 commits September 12, 2013 13:13

Semi-functional geocoding widget.

b44521a

Animate to destination instead of jumping there.

1d93eae

Style improvements, make enter submit.

fce2533

Show resolved name after geocoding.

63c7c36

Cleanup.

6e579cd

Visualize routes between two points.

0758e5a

Update clock to match route.

bb8b727

Put nav routes on terrain. Also temp demo app.

cd67e54

Add shady depth test toggle.

554919e

Elimiante some wackyness in the route.

d8bc6f6

Show a marker instead of a point.

2654aa9

Move geocoding widget to the top right.

a3f9d83

GeocodingWidget doc.

020bbdf

Move geocoding widget above buttons.

649271f

Merge remote-tracking branch 'origin/bingKey' into geocodingWidget

e120201

Clean up use of Bing Maps API.

c9c261f

Merge remote-tracking branch 'origin/master' into geocodingWidget

7909e9b

Merge remote-tracking branch 'origin/bingKey' into geocodingWidget

65c85c0

Conflicts: Source/Core/BingMapsApi.js

Rip out route plotting functionality, and some cleanup.

f5bf367

Fix destroy, and add tests.

f597cdb

Another test of the GeocodingWidget's viewmodel.

9958b61

Remove Sandcastle app used for demoing.

d496ed3

Update changes.md.

d7bc870

Remove unused file.

417010b

pjcozzi mentioned this pull request Sep 19, 2013

Long camera flights #1158

Closed

mramato added 3 commits October 22, 2013 21:24

Updates after review

2e803ca

Fix CHANGES Move `padding-left` to wide and hover classes so that collapsed version is the right size Add `hasFocus` binding and document listeners so that the text box disappears in all expected situations. Cleanup knockout property declarations.

Workaroud apparent knockout bug

f503dd7

Using the `hasFocus` binding with a knockout-es5 property seems to cause issues. Instead, bind directly to a low-level observable.

Add missing Geocoder specs to Viewer and fix issues it uncovered.

d670d2c

Viewer.destroy needs to destroy the Geocoder.

c3023c5

Flesh out unit tests, fix uncovered bugs.

777488e

mramato added 2 commits October 23, 2013 09:26

Improve focus handling.

41132ab

For some reason this still doesn't work as expected on mobile. I think our touch handling in the cameara may be eating the event.

Make sure searchText is always a string, fix specs.

0ab0bed

Make the CSS a bit clearer. No change to functionality.

2d4386b

emackey reviewed Oct 23, 2013
View reviewed changes

Fix jsDoc.

7e3e47f

Fix clicking button to search.

67ce49e

shunter added a commit that referenced this pull request Oct 23, 2013

Merge pull request #1157 from AnalyticalGraphicsInc/geocodingWidget

21ee7f8

Geocoding widget

shunter merged commit 21ee7f8 into master Oct 23, 2013

shunter deleted the geocodingWidget branch October 23, 2013 15:36

mramato mentioned this pull request Oct 23, 2013

Geocoder improvements #1262

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Geocoding widget #1157

Geocoding widget #1157

kring commented Sep 18, 2013

mramato commented Sep 18, 2013

kring commented Sep 19, 2013

pjcozzi commented Sep 19, 2013

pjcozzi commented Sep 19, 2013

kring commented Sep 19, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey Oct 23, 2013

mramato Oct 23, 2013

emackey commented Oct 23, 2013

shunter commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey commented Oct 23, 2013

mramato commented Oct 23, 2013

Geocoding widget #1157

Geocoding widget #1157

Conversation

kring commented Sep 18, 2013

mramato commented Sep 18, 2013

kring commented Sep 19, 2013

pjcozzi commented Sep 19, 2013

pjcozzi commented Sep 19, 2013

kring commented Sep 19, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey Oct 23, 2013

Choose a reason for hiding this comment

mramato Oct 23, 2013

Choose a reason for hiding this comment

emackey commented Oct 23, 2013

shunter commented Oct 23, 2013

mramato commented Oct 23, 2013

mramato commented Oct 23, 2013

emackey commented Oct 23, 2013

mramato commented Oct 23, 2013