pbugnion / gmaps

Google maps for Jupyter notebooks
https://jupyter-gmaps.readthedocs.io/en/stable/
Other
760 stars 146 forks source link

Add tilt option to gmaps.figure #253

Closed razzius closed 6 years ago

razzius commented 6 years ago

tilt is an option provided by the Google Maps JavaScript API, but prior to this has not been exposed by this library.

image

Please excuse the unpolished code - I expect it will take some editing to get this feature ready for merging, but I figured the earlier I could get feedback on my overall approach, the better.

In particular I don't really know what I'm doing with traitlets, and perhaps tilt should default to None rather than 45 and the JavaScript setTilt call should only happen if tilt is specified.

Furthermore, since tilt doesn't apply to all maps types, gmaps could intelligently validate that setting tilt makes sense for the map type requested (although Google Maps itself accepts setTilt calls for all map types and ignores them if they don't apply).

pbugnion commented 6 years ago

This looks great! Thanks very much.

One thing that's currently missing is the ability to dynamically set tilt. For instance, if I run:

fig = gmaps.figure(center=(37.743, -122.428), zoom_level=20, map_type='SATELLITE', tilt=45)
fig  # display figure

... then in a new cell:

fig.map_type = 'TERRAIN'

... this changes the map type in the rendered map. Being able to set options dynamically is important for putting maps in larger ipywidgets applications.

Fortunately, traitlets and the widgets communication protocols takes care of a lot of the work for you. In Map.js, you can define callbacks that run when the model changes Python-side. See, for instance, what we do with map_type or mouse_handling here. Typically, in gmaps, the _modelEvents method defines the callbacks on model changes.

perhaps tilt should default to None rather than 45 and the JavaScript setTilt call should only happen if tilt is specified.

I think what you've done is fine: 45 is the default in Google Maps, so surfacing that explicitly in Python makes it easier for users to see the default (e.g. in method signatures etc.)

Besides that, I've made a couple of comments inline. Looks great! Thanks again!

razzius commented 6 years ago

Took my best attempt at making the changes you suggested and things seem to work. In fact there's even a little animation effect that comes with Google Maps when you change the tilt in later cells!

Thanks for your comments, and thanks for providing this library!

pbugnion commented 6 years ago

Thanks very much for making those changes, and thanks again for the PR. At first glance the changes look good, but I want to give this another proper whirl tomorrow morning BST.

Thanks again!

pbugnion commented 6 years ago

Thanks -- I made a tiny tweak to the docstring because it was rendering strangely in Sphinx.

I'll push out a new release with this change over the next couple of days.

Looks great, thanks again!