-
Notifications
You must be signed in to change notification settings - Fork 1
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
wip: created osm and googlemap providers classes, added env support #50
base: main
Are you sure you want to change the base?
Conversation
Even if OSM doesn't have support for dark mode, we need to provide a common interface for all callers
@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)
Tasks Completed:
.env
levelGoogleMapProvider
&OpenStreetMapProvider
classes as providers.Issues In-progress: