-
Notifications
You must be signed in to change notification settings - Fork 4
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 support for OpenStreetMap #50
Conversation
@tarunsinghofficial this is looking good! I noticed a few issues and areas for improvement, though:
I think the approach you're taking of creating the map provider inside of the MapView (which I renamed, by the way), is fine for the moment but I would recommend considering a dependency injection approach instead. i.e. the parent component to MapView figures out the correct map provider and gives that to MapView. |
a7eb77c
to
1668df7
Compare
Hi @aaronbrethorst, I have made the required changes. |
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 moving in the right direction! I like how you've factored the Google and OSM code into different providers, and I think the architecture looks good. I am seeing a few behavioral issues that will need to be fixed before this can be merged:
- Markers not loading on the OSM map (see below for the relevant console errors)
- Leaflet zoom controls need to be in the same location as the Google zoom controls (bottom right)
OpenStreetMapProvider.js:34 Uncaught (in promise) TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
at OpenStreetMapProvider.addMarker (OpenStreetMapProvider.js:34:13)
at addMarker (MapView.svelte:137:33)
at MapView.svelte:107:27
at Array.forEach (<anonymous>)
at $$self.$$.update (MapView.svelte:107:12)
at update (chunk-5RNXQC66.js?v=4f5260ef:1319:8)
at flush (chunk-5RNXQC66.js?v=4f5260ef:1290:9)
Markers not loading on the OSM map (see below for the relevant console errors) Fixed ✅ |
Hey @aaronbrethorst! Sorry for not being active for some time. I have been actively preparing for my interviews for placements, so I didn't have the time to continue working on this PR. But soon I will be contributing to the project. Hope you understand :) Thanks |
Last updates: Display route on OSM map and refactor Route Component to adapt the plug-in architecture ✅ I think we're ready to merge after review |
no worries @tarunsinghofficial |
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.
awesome work! 🚀
* Created osm and googlemap providers classes, added the support at env * added global to GoogleMapProvider.js * linting: removed theme, type from OpenStreetMapProvider.js
* Renames GoogleMap.svelte to MapView.svelte * Adds setTheme() to the OSM provider - Even if OSM doesn't have support for dark mode, we need to provide a common interface for all callers * Add `$config` alias * Add some TODOs * Delete duplicate code * Linting fixes
* Move new StopMarker() into GoogleMapProvider * feat: added the dependency injection approach for map provider * fixes: map switch issue, removed unncessary imports, mapview * fixes: error thrown on zooming and loading more stops * linting
* feat: Add stops markers * style: Add styles for leaflet map controls * fix: route display in google maps * refactor: Remove unused code * style: Add z-index to map type button * refactor: simplify marker rendering by removing redundant google maps Marker * refactor: removed unused variable * feat: Add polyline functionality to OpenStreetMapProvider * feat: Add MapSource configuration file * refactor: adapt plugin architecture for multiple maps, add OpenStreetMap support * feat: Add PopupContent component for displaying stop information * refactor: Update MapView component to pass mapSource prop to RouteMap * refactor: Update MapContainer to pass mapSource prop to MapView * refactor: Add leaflet-polylinedecorator and polyline-encoded packages * refactor: Add 'L' as a readonly global variable in eslint.config.js
e8b1eb5
to
23aadc3
Compare
Tasks Completed:
.env
levelGoogleMapProvider
&OpenStreetMapProvider
classes as providers.Issues In-progress: