Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pydeck: Add Google Maps base maps to pydeck #4632

Merged
merged 7 commits into from
Jun 4, 2020

Conversation

ajduberstein
Copy link
Collaborator

@ajduberstein ajduberstein commented Jun 2, 2020

For #4323

Background

Support the addition of the Google Maps base map to pydeck

Works exclusively via .to_html for now, since I can add the Google Maps JS library as a script tag. Where is the most convenient place to fetch the library for the Jupyter transport module--as a custom library?

Change List

  • Add @deck.gl/google-maps to jupyter-widget's bundle
  • Add Python tests
  • Include a parameter to allow switching the base map within pydeck
  • Read Google Maps token in the same way the Mapbox token is read

@ajduberstein ajduberstein mentioned this pull request Jun 2, 2020
7 tasks
@ajduberstein ajduberstein requested a review from ibgreen June 2, 2020 09:18
@ajduberstein
Copy link
Collaborator Author

image

@coveralls
Copy link

coveralls commented Jun 2, 2020

Coverage Status

Coverage increased (+2.7%) to 83.212% when pulling 0d0eb6b on ajd/google-maps-in-pydeck into 89bd3c0 on master.

@@ -2,6 +2,9 @@
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8" />
<title>pydeck</title>
{% if google_maps_key %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer we inject the google maps API programmatically (loadScript) rather than through a template injection. That would make this a much more portable part of the playground app.

We could just make a note of this in a tracker task and do that as a separate second PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted here #4644

container
});
const {mapProvider} = props;
if (mapProvider === 'mapbox') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: prefer switch statement in this situation:

switch (mapProvider) {
  case 'mapbox': ... break;
  case 'google': 

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose we could even add arcgis here now if we wanted too...

getTooltip,
container
});
} else if (mapProvider === 'google_maps') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Provider = google_maps or just google?

I suppose google_maps is more consistent with googleMapsKey etc

) {
switch (mapProvider) {
case 'mapbox':
console.debug('Using Mapbox base maps');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use log from @deck.gl/core

}) {
googleMapsKey = props.googleMapsKey || googleMapsKey;
if (!googleMapsKey) {
deck.log.warn('No Google Maps API key set');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This returns a function. You need to call it: log.warn(...)()

Make sure that you test this use case.

version "3.0.0"
resolved "https://registry.yarnpkg.com/@jupyter-widgets/base/-/base-3.0.0.tgz#3996d566cddb742d275b007e0713de1e17f55a44"
integrity sha512-un1ZTHALCwE/SAYk2gEaonYM1JoaFyhosN8a3y2bhl4N26yCB3dP1PqGHLsAFur6ZB7fwuWwBEkIZx+nOwstAQ==
"@jupyter-widgets/base@^2":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this changed without a corresponding change in package.json?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know, but yarn bootstrap also resolves this to 2.0.2 for me now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is my fault. Somehow in #4573 I moved @jupyter-widgets/base to peerDependencies but either forgot to remove it from dependencies or rebased incorrectly or something.

So I think I had previously built and pushed the yarn.lock having removed @jupyter-widgets/base from dependencies, and it resolved to 3.0.0.

@ajduberstein ajduberstein merged commit e4af87e into master Jun 4, 2020
@ajduberstein ajduberstein deleted the ajd/google-maps-in-pydeck branch June 4, 2020 00:32
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.

None yet

5 participants