-
Notifications
You must be signed in to change notification settings - Fork 39
wip: Define models for Views, Basemap #908
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a small separate refactor to make it easier to read _map.py
by removing the HTML export functionality
_esm = bundler_output_dir / "index.js" | ||
_css = bundler_output_dir / "index.css" | ||
|
||
_has_click_handlers = t.Bool(default_value=False, allow_none=False).tag(sync=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is from moving view_state
lower so that it's immediately after the new views
attribute; nothing was changed except for adding views
basemap_style = BasemapUrl(CartoBasemap.PositronNoLabels) | ||
""" | ||
A URL to a MapLibre-compatible basemap style. | ||
basemap = t.Instance(MaplibreBasemap, allow_none=True).tag( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows users to customize the basemap, and to turn off the basemap altogether by passing None
.
Before merging we may keep basemap_style
around to not break backwards compatibility, but I think documenting that the new API is
Map(basemap=MaplibreBasemap(basemap_style=basemap_style))
isn't too bad
mode: Literal[ | ||
"interleaved", | ||
"overlaid", | ||
"reverse-controlled", | ||
] = "reverse-controlled", | ||
basemap_style: str | CartoBasemap, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the user can explicitly choose a rendering mode between interleaved
(mapbox overlay set to interleaved), overlaid
(mapbox overlay not using interleaved), or reverse-controlled
, the existing behavior, which doesn't use mapbox overlay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file implements Python models for deck.gl Views, allowing a definition of a globe view and of non-geospatial views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This defines the TS/JS side of the view models, with the ability to create deck View
objects
@kylebarron The general approach seems quite comprehensive. I’d suggest implementing the different modes more progressively, as this will likely require some refactoring on the JS side. In hindsight, adding XState wasn’t the best choice on my part, it makes state management a bit harder to evolve. I’ll open a ticket to propose an alternative. I think we can still address this technical debt while it’s not too complex, but not on this PR. |
I think despite what are a bunch of changes here on the Python side, the JS side only needs to change to support two modes: One with mapbox overlay and one without. We can also merge this PR or something similar without exposing the classes to users, so we can keep developing on the JS side |
@kylebarron sounds good. Now that we merged the Playwright PR we can add specs to ensure JS features continue to work as expected. |
@vgeorge @felixpalmer this is progress on the Python side to be able to serialize
In particular, #906 passes a bare
projection="globe"
parameter through. I think this is too simplistic because there are too many other things to consider when switching to globe view.This PR defines all the data models for deck.gl views and maplibre basemap information so that on the JS side we should be able to precisely map the input into deck.gl calls.
This PR also sets the stage for non-geospatial map rendering and for multi-view support.
With this PR, to generate a map with globe view, someone would call
Perhaps we could add something like
projection="globe"
toviz
to permit a less-verbose way for users to define a map with a globe, but that would call the above code internally on the Python side.Relates to
ipywidgets
#905 (multi-views),TODO:
before_id
attribute to layers