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

Add Leaflet module #138

Merged
merged 1 commit into from
Sep 17, 2024
Merged

Add Leaflet module #138

merged 1 commit into from
Sep 17, 2024

Conversation

zakjan
Copy link
Collaborator

@zakjan zakjan commented Sep 14, 2024

No description provided.

Copy link
Contributor

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

Very nice indeed. Some minor nits, feel free to ignore based on what makes sense to you. Plus a suggestion to break out some changes into separate PR.

import 'leaflet/dist/leaflet.css';

// source: Natural Earth http://www.naturalearthdata.com/ via geojson.xyz
const AIR_PORTS =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const AIR_PORTS =
const AIRPORTS_URL =

return this._deck.pickObjects(opts);
}

#getMap(): L.Map & {_animatingZoom: boolean; _getMapPanePos: () => L.Point} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: are these private declarations? We usually use the typescript private keyword and a leading underscore

@@ -12,6 +12,7 @@
},
"type": "module",
"workspaces": [
"examples/*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be good to do this and the bing maps fix in separate PRs. This one in particular has caused some issues in the past. Make sure you can build the website. May have to revert if it messes up publishing.

@@ -0,0 +1,12 @@
// deck.gl-community
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine, though not super excited about files called index... maybe we could name it after the tests inside?

@ibgreen
Copy link
Contributor

ibgreen commented Sep 16, 2024

@zakjan Feel free to merge whenever you are ready, or let me know if you need me to merge.

@ibgreen ibgreen merged commit bfb1e85 into visgl:master Sep 17, 2024
1 check passed
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.

2 participants