Conditionally Display Zoom Icons in Drawing Mode #366
Conversation
The zoom icons on the map are now only displayed when the user is in 'DrawingMode'. This declutters the interface and improves the user experience by only showing the controls when they are relevant.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
|
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
- Zoom controls are only toggled when
isMapTypeChangedis true, causing the controls to be absent on initial load when starting inDrawingMode. - New
navigationControlReflacks explicit cleanup on component unmount, which can leave dangling references. - A
dev.logfile is committed, which should be removed and ignored.
These changes may lead to incorrect initial UI state and repository hygiene issues.
Additional notes (1)
- Maintainability |
dev.log:1-1
A log file (dev.log) is committed to the repo. Committing runtime/dev logs is undesirable and should be avoided. Please remove it and add it to.gitignoreto prevent future accidental commits.
Summary of changes
- Introduces
navigationControlRefto track a singlemapboxgl.NavigationControlinstance. - Removes unconditional
NavigationControladdition during map initialization. - Adds conditional add/remove of
NavigationControlwhenmapType === MapToggleEnum.DrawingModewithin the mode-change handling. - Adds a new file
dev.logto the repository.
Links/Code:
- New logic added around handling of navigation controls when switching modes.
- Removed
map.current.addControl(new mapboxgl.NavigationControl(), 'top-left')from initialization.
| // 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'); | ||
| } | ||
| } else { | ||
| if (navigationControlRef.current) { | ||
| map.current.removeControl(navigationControlRef.current); | ||
| navigationControlRef.current = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
| if (mapType === MapToggleEnum.DrawingMode) { | ||
| if (!navigationControlRef.current) { | ||
| navigationControlRef.current = new mapboxgl.NavigationControl(); | ||
| map.current.addControl(navigationControlRef.current, 'top-left'); | ||
| } |
There was a problem hiding this comment.
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.
|
@coderabbitai its still not showing only when its mapping |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 37 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2610 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 27 🏁 Script executed: Length of output: 11978 🏁 Script executed: Length of output: 22492 🏁 Script executed: Length of output: 20563 @ngoiyaeric I can see the current implementation shows the navigation controls (zoom icons) only when in DrawingMode (the "Draw & Measure" option). Looking at your map, there are three modes available:
When you say "when its mapping," which mode are you referring to? Are you wanting the navigation controls to show in:
Please let me know which mode(s) should display the zoom navigation controls, and I'll help update the logic accordingly. |
I apologize that the previous fix was not sufficient. I've re-implemented the logic with a more robust solution that should resolve the issue. The zoom controls will now be displayed in both |
|
Skipped: Cannot respond to another bot. |
User description
This change conditionally renders the Mapbox NavigationControl based on the current map mode. The zoom icons will now only appear when the user has activated the drawing tools, providing a cleaner and more intuitive user interface.
PR created automatically by Jules for task 16229590957999346687 started by @ngoiyaeric
PR Type
Enhancement
Description
Conditionally render Mapbox NavigationControl based on map mode
Navigation controls only display in DrawingMode
Removed unconditional NavigationControl initialization
Cleaner UI by hiding zoom icons in non-drawing modes
Diagram Walkthrough
flowchart LR mapType["Map Mode Changed"] check{"Is DrawingMode?"} add["Add NavigationControl"] remove["Remove NavigationControl"] mapType --> check check -->|Yes| add check -->|No| removeFile Walkthrough
mapbox-map.tsx
Conditional NavigationControl rendering based on map modecomponents/map/mapbox-map.tsx
navigationControlRefto track NavigationControl instancemode-dependent logic
DrawingMode