-
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
[feat] Hiding Markers when Search and Filters are Activated #86
base: main
Are you sure you want to change the base?
[feat] Hiding Markers when Search and Filters are Activated #86
Conversation
@@ -87,8 +89,16 @@ export const MarkerInfoWindow = ({ | |||
useEffect(() => { | |||
if (marker && clusterer) { | |||
clusterer.addMarker(marker); | |||
markerMap.set(projectId, marker as unknown as google.maps.Marker); |
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 bypasses the type-checking system but may cause runtime issues bc of compatibility. marker
= AdvancedMarkerElement but markerMap
takes in google.maps.Marker which is needed to call setMap()
.
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.
hmmm, I don't want to run the risk of runtime errors and when I checked it on my VSCode it did say Marker is depreciated. Since this feature does not need to be merged in ASAP, let's work together on a fix after Thanksgiving Break.
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.
Great job on this sprint Neha! I think since Marker could cause compatibility issues we will need to rework this a bit after the break so don't merge it for now
@@ -129,6 +130,50 @@ export default function AddMarker({ | |||
return setClusterer; | |||
}, [map]); | |||
|
|||
const markerMap = useRef<Map<number, google.maps.Marker>>(new Map()); |
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.
What does this line do?
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 creates markerMap
which is like a hashmap that maps the project ids to its corresponding marker (google.maps.Marker). useRef
is just a hook that makes it so that the component doesn't re-render every time we update the map. Instead, useRef
creates a mutable object (markerMap.current) that exists across re-renders of the component.
@@ -87,8 +89,16 @@ export const MarkerInfoWindow = ({ | |||
useEffect(() => { | |||
if (marker && clusterer) { | |||
clusterer.addMarker(marker); | |||
markerMap.set(projectId, marker as unknown as google.maps.Marker); |
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.
hmmm, I don't want to run the risk of runtime errors and when I checked it on my VSCode it did say Marker is depreciated. Since this feature does not need to be merged in ASAP, let's work together on a fix after Thanksgiving Break.
What's new in this PR
Description
markerMap
to map the project ids to its corresponding marker.Screenshots
Screen.Recording.2024-11-24.at.8.14.44.PM.mov
How to review
Next steps
google.maps.Marker
vsgoogle.maps.marker.AdvancedMarkerElement
->setMap()
is part of Marker not AdvancedMarkerElement (check comment)Relevant links
Online sources
Related PRs
CC: @itsliterallymonique