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

Ecoscope fork merge #613

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from
Draft

Ecoscope fork merge #613

wants to merge 31 commits into from

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Aug 27, 2024

This PR addresses issue #592. The aim is to integrate changes from the ecoscope fork of Lonboard and discuss their potential inclusion in the main project.

The fork was initiated in April 2024. While the front-end has evolved since then, the changes appear manageable for integration. Current commit resolves merge conflicts and includes a minor adjustment to ensure map component rendering.

Below is a summary of changes from the remote repository. Each item requires review before consideration for merging:

  • v9 bump
  • Add mapWidth to Map
  • Add support for Deck widgets
  • Implement an alternative visualization that returns a layer instead of a map
  • Surface deck.gl MapView in index.tsx (to enable repeat=true)
  • Expose controller on the Python side in Map

@kylebarron @batpad please let me know if you have comments.

Copy link
Member

Choose a reason for hiding this comment

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

This is generated code and should not be checked in.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto, this is also generated

@@ -13,6 +14,7 @@ class MapKwargs(TypedDict, total=False):
_height: int
basemap_style: Union[str, CartoBasemap]
parameters: Dict[str, Any]
deck_widgets: List[BaseDeckWidget]
Copy link
Member

Choose a reason for hiding this comment

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

This should be Sequence (or maybe Iterable) instead of List

@@ -17,6 +17,8 @@
"framer-motion": "^11.3.30",
"lodash.debounce": "^4.0.8",
"lodash.throttle": "^4.1.1",
"@deck.gl/widgets": "^9.0.16",
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we alphabetically sort our dependencies? I'd like to visually keep track of deck.gl dependencies next to each other.

Also, all deck.gl libraries should have the same version constraint.

import { CompassWidget, ZoomWidget, FullscreenWidget, LightTheme } from "@deck.gl/widgets";
import { LegendWidget, NorthArrowWidget, TitleWidget, ScaleWidget, SaveImageWidget } from "./deck-widget.js";

export abstract class BaseDeckWidgetModel extends BaseModel {
Copy link
Member

Choose a reason for hiding this comment

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

At first glance, I wouldn't think these should be models... do the widgets themselves need to be reactive on the Jupyter side?

@@ -220,6 +220,48 @@ def viz(

return Map(layers=layers, **map_kwargs)

def viz_layer(
Copy link
Member

Choose a reason for hiding this comment

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

If we add viz_layer, then we should change viz() to use viz_layer under the hood, because it looks like it's entirely duplicated.

Separately, I want to think over this API a bit more.

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.

3 participants