-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Clean up basemap-browser example per review feedback #9934
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
Conversation
Co-authored-by: chrisgervang <[email protected]>
examples/basemap-browser/README.md
Outdated
| - ✅ Pure JS + Interleaved: false | ||
| - ✅ React + Interleaved: true | ||
| - ✅ React + Interleaved: false | ||
| | Provider | Framework | Interleaved | Status | |
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.
Remove status column
examples/basemap-browser/README.md
Outdated
| ## Architecture | ||
|
|
||
| The basemap-browser uses TypeScript and React function components with a modular architecture: | ||
| The basemap-browser uses a two-React-roots architecture with TypeScript throughout: |
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 sentence doesn't make sense
| "@deck.gl/layers": "*", | ||
| "@deck.gl/mapbox": "*", | ||
| "@vis.gl/react-google-maps": "^1.7.1", | ||
| "@vis.gl/react-google-maps": "1.7.1", |
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.
I didn't mean you needed to pin react-google-maps. I meant you needed to pin deck.gl packages
| // eslint-disable-next-line no-process-env | ||
| const MAPBOX_TOKEN = process.env.MapboxAccessToken; | ||
|
|
||
| // Mapbox Overlay wrapper |
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.
Remove comment
|
|
||
| import 'maplibre-gl/dist/maplibre-gl.css'; | ||
|
|
||
| // MapLibre Overlay wrapper |
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.
Remove comment
|
@copilot please take a look at my comments |
Co-authored-by: chrisgervang <[email protected]>
All comments addressed in commit d6b69d2:
|
examples/basemap-browser/README.md
Outdated
| ## Architecture | ||
|
|
||
| The basemap-browser uses TypeScript and React function components with a modular architecture: | ||
| The basemap-browser uses a two-React-roots architecture: |
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 still isn't true or a correct summary of the architecture.
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.
Fixed in commit 2990945. Changed to "This example demonstrates both Pure JS and React integration patterns with deck.gl" which accurately describes the application structure.
| "@deck.gl/google-maps": "*", | ||
| "@deck.gl/layers": "*", | ||
| "@deck.gl/mapbox": "*", | ||
| "@deck.gl/core": "9.0.33", |
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.
These deck modules need to be the latest 9.2
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.
Updated to 9.2.0-beta.4 in commit 2990945.
|
@copilot I left a review |
Co-authored-by: chrisgervang <[email protected]>
| "@deck.gl/google-maps": "*", | ||
| "@deck.gl/layers": "*", | ||
| "@deck.gl/mapbox": "*", | ||
| "@deck.gl/core": "9.2.0-beta.4", |
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.
It's ^9.2.0
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.
Updated to ^9.2.0 in commit aa849e3.
| "@deck.gl/mapbox": "9.2.0-beta.4", | ||
| "@vis.gl/react-google-maps": "^1.7.1", | ||
| "deck.gl": "*", | ||
| "deck.gl": "9.2.0-beta.4", |
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.
Why do we need this when we're already adding submodules?
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.
Removed in commit aa849e3. The code only imports from @deck.gl/* submodules, so the main deck.gl package was redundant.
|
@copilot I've left comments |
…pendency Co-authored-by: chrisgervang <[email protected]>
Background
Address code review feedback on the basemap-browser example application.
Change List
Naming & Consistency
MapboxOverlayWrapper→MapboxDeckOverlay,MapLibreOverlay→MapLibreDeckOverlay^9.2.0for deck.gl submodule packages (@deck.gl/core,@deck.gl/google-maps,@deck.gl/layers,@deck.gl/mapbox)deck.glpackage dependency (code only uses individual submodules)Cleanup & Optimization
deckOverlay.finalize()in mapbox/maplibre Pure JS implementations (map.remove() handles cleanup via onRemove)Configuration & Documentation
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Note
Streamlines the basemap-browser example and tightens docs/config.
@deck.gl/*to^9.2.0, removedeck.gl, addvitedev dep inpackage.jsonMapboxDeckOverlayandMapLibreDeckOverlaydeckOverlay.finalize()in Pure JSmapbox.ts/maplibre.ts; add cancellation guard toexamples-pure-js/google-maps.tsuseEffecttsconfig.jsonnow extends root and trims optionsREADME.mdarchitecture and format test matrix as a tableWritten by Cursor Bugbot for commit aa849e3. This will update automatically on new commits. Configure here.