Skip to content

Conditionally Display Zoom Icons in Drawing Mode #366

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.

Sign up for GitHub

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

Open
google-labs-jules wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conditionally Display Zoom Icons in Drawing Mode #366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions components/map/mapbox-map.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
const { mapData, setMapData } = useMapData(); // Consume the new context, get setMapData
const { setIsMapLoaded } = useMapLoading(); // Get setIsMapLoaded from context
const previousMapTypeRef = useRef<MapToggleEnum | null>(null)
const navigationControlRef = useRef<mapboxgl.NavigationControl | null>(null);
Copy link

@charliecreates charliecreates bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You added a persistent navigationControlRef, but there is no corresponding unmount/cleanup path shown for removing the control if the map is destroyed. While Mapbox cleans up on map.remove(), explicitly removing the control prevents potential dangling references and keeps the lifecycle clear.

Suggestion

Add cleanup to remove the control during component unmount or before calling map.remove():

// In your cleanup useEffect
if (map.current && navigationControlRef.current) {
  map.current.removeControl(navigationControlRef.current);
  navigationControlRef.current = null;
}

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.


// Refs for long-press functionality
const longPressTimerRef = useRef<NodeJS.Timeout | null>(null);
Expand Down Expand Up @@ -351,6 +352,19 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number

// Handle geolocation setup based on mode
setupGeolocationWatcher()

// Handle navigation controls based on mode
if (mapType === MapToggleEnum.DrawingMode) {
if (!navigationControlRef.current) {
navigationControlRef.current = new mapboxgl.NavigationControl();
map.current.addControl(navigationControlRef.current, 'top-left');
}
Comment on lines +357 to +361
Copy link

@charliecreates charliecreates bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intent is to show only zoom icons (as the PR title/description states), the default NavigationControl will also render the compass. You should explicitly configure it to hide the compass.

Suggestion

Replace the constructor with options to hide the compass and show only zoom:

navigationControlRef.current = new mapboxgl.NavigationControl({
  showCompass: false,
  showZoom: true,
});

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.

} else {
if (navigationControlRef.current) {
map.current.removeControl(navigationControlRef.current);
navigationControlRef.current = null;
}
}
Comment on lines +356 to +367
Copy link

@charliecreates charliecreates bot Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The navigation control toggling is gated by isMapTypeChanged. This can cause the zoom icons to never appear on initial load when the app starts in DrawingMode (because no mode change has occurred yet and the unconditional add during init was removed). This is a functional regression relative to the previous behavior and violates the stated goal of showing zoom icons whenever drawing tools are active.

Recommend handling the control setup regardless of whether the mode changed (idempotent due to the ref), or add an initial check after map initialization to reflect the current mode.

Suggestion

Move the navigation control toggling outside of the isMapTypeChanged branch so it runs whenever the effect executes and the map is initialized, e.g.:

// After ensuring map.current and initializedRef.current
if (mapType === MapToggleEnum.DrawingMode) {
  if (!navigationControlRef.current) {
    navigationControlRef.current = new mapboxgl.NavigationControl();
    map.current.addControl(navigationControlRef.current, 'top-left');
  }
} else if (navigationControlRef.current) {
  map.current.removeControl(navigationControlRef.current);
  navigationControlRef.current = null;
}

Alternatively, add a one-time check in the map.current.on('load', ...) handler to add the control if the initial mode is DrawingMode.

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.


// Handle draw controls based on mode
if (mapType === MapToggleEnum.DrawingMode) {
Expand Down Expand Up @@ -409,8 +423,6 @@ export const Mapbox: React.FC<{ position?: { latitude: number; longitude: number
preserveDrawingBuffer: true
})

map.current.addControl(new mapboxgl.NavigationControl(), 'top-left')

// Register event listeners
map.current.on('moveend', captureMapCenter)
map.current.on('mousedown', handleUserInteraction)
Expand Down
Empty file added dev.log
View file
Open in desktop
Empty file.