[REFACTOR] naver.map.Event 적용 #80
Conversation
Walkthrough이 변경 사항은 Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Update: 2024년 11월 28일 03시 29분 12초 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/utils/naver/currentMarkerToMap.ts (1)
27-29: 줌 레벨을 구성 가능하게 만들어보세요현재 줌 레벨이 18로 하드코딩되어 있습니다. 다양한 사용 사례에 대응하기 위해 매개변수로 받는 것이 좋을 것 같습니다.
다음과 같이 변경을 제안합니다:
export const currentMarkerToMap = ( position: { lat: number; lng: number }, mapInstance: naver.maps.Map, - currentLocationMarkerRef: React.MutableRefObject<naver.maps.Marker | null> + currentLocationMarkerRef: React.MutableRefObject<naver.maps.Marker | null>, + zoomLevel: number = 18 ) => { // ... existing code ... naver.maps.Event.once(mapInstance, 'idle', () => { - mapInstance.setZoom(18); + mapInstance.setZoom(zoomLevel); }); };src/utils/naver/addMarkerToMap.ts (1)
56-57: 부드러운 이동 구현에 대한 개선 제안현재 구현은 잘 작동하지만, 다음과 같은 개선사항을 고려해보시면 좋을 것 같습니다:
- 이동 거리에 따른 동적인 애니메이션 시간 계산
- 줌 레벨 변경과 이동이 동시에 일어날 때의 사용자 경험 최적화
다음과 같은 구현을 제안드립니다:
- mapInstance.current?.panTo(center, { duration: 500, easing: 'easeOutCubic' }); + const currentCenter = mapInstance.current?.getCenter(); + const distance = currentCenter ? getDistance(currentCenter, center) : 0; + const duration = Math.min(Math.max(distance * 100, 300), 1000); // 300ms to 1000ms + mapInstance.current?.panTo(center, { duration, easing: 'easeOutCubic' });src/components/features/NaverMap/index.tsx (1)
Line range hint
1-71: PR 목표에 명시된 panTo 구현이 누락되었습니다.PR 목표에서 언급된
setCenter와setZoom을panTo로 전환하는 작업이 구현되지 않았습니다. 현재 코드에서는 여전히 정적인 방식으로 지도 중심점을 설정하고 있습니다.다음과 같은 개선을 제안드립니다:
useEffect(() => { if (mapRef.current && window.naver && !mapInstance.current) { const center = new window.naver.maps.LatLng(initialCenter.lat, initialCenter.lng); const map = new window.naver.maps.Map(mapRef.current, { center, zoom: initialZoom, draggable: true, }); const listener = window.naver.maps.Event.addListener(map, 'zoom_changed', () => { const currentZoom = map.getZoom(); handleZoomChange(currentZoom); }); mapInstance.current = map; return () => { window.naver.maps.Event.removeListener(listener); mapInstance.current = null; }; } }, [initialZoom, handleZoomChange, initialCenter.lat, initialCenter.lng]); +const moveToLocation = useCallback((lat: number, lng: number) => { + if (mapInstance.current) { + const position = new window.naver.maps.LatLng(lat, lng); + mapInstance.current.panTo(position, { duration: 500, easing: 'easeOutCubic' }); + } +}, []); useEffect(() => { clearMarkers(); if (mapInstance.current && markers.length > 0) { addMarkerToMap(mapInstance, markers, markersRef, markerDataRef); + // 첫 번째 마커 위치로 부드럽게 이동 + if (markers[0]) { + moveToLocation(markers[0].lat, markers[0].lng); + } } if (mapInstance.current && isCurrent && markers.length === 0) { currentMarkerToMap(initialCenter, mapInstance.current, currentLocationMarkerRef); + moveToLocation(initialCenter.lat, initialCenter.lng); } -}, [clearMarkers, initialCenter, isCurrent, markers]); +}, [clearMarkers, initialCenter, isCurrent, markers, moveToLocation]);src/pages/MapView.tsx (2)
35-41: 에러 처리 로직 추가를 고려해주세요.
handleDataUpdate함수에 다음과 같은 개선사항을 제안드립니다:
- 데이터 유효성 검증
- 에러 발생 시 사용자 피드백
const handleDataUpdate = useCallback( (newData: Place[], type: BottomSheetType) => { + if (!Array.isArray(newData)) { + console.error('Invalid data format'); + return; + } clearMarkers(); newData?.forEach(addMarker); setState({ data: newData, type }, token); }, [clearMarkers, addMarker, setState, token] );
44-49: 클린업 함수 추가를 검토해주세요.
useEffect에 마커 정리를 위한 클린업 함수 추가를 권장드립니다.useEffect(() => { const extractedData = location.state?.data; if (extractedData?.places.length) { handleDataUpdate(extractedData.places, 'extract'); } + return () => { + clearMarkers(); + }; }, [handleDataUpdate, location.state, setState, token]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/components/features/NaverMap/index.tsx(1 hunks)src/pages/MapView.tsx(4 hunks)src/utils/naver/addMarkerToMap.ts(2 hunks)src/utils/naver/currentMarkerToMap.ts(1 hunks)
🔇 Additional comments (8)
src/utils/naver/currentMarkerToMap.ts (1)
24-26: 애니메이션 구현이 잘 되었습니다!
panTo 메서드와 duration 설정을 통해 부드러운 애니메이션을 구현한 것이 좋습니다. PR 목표에 맞게 setCenter에서 panTo로 성공적으로 전환되었습니다.
src/utils/naver/addMarkerToMap.ts (2)
11-11: 마커 데이터 참조 추가가 적절해 보입니다!
마커의 메타데이터를 저장하기 위한 ref 추가가 잘 구현되어 있습니다.
52-54: 이벤트 핸들링 로직 검증이 필요합니다.
연속적인 호출이 발생할 경우나 컴포넌트 언마운트 시의 동작을 확인해주세요. 현재 구현에서 다음 사항들을 고려해볼 필요가 있습니다:
- 연속적인 호출 시 이벤트 리스너의 중복 등록 가능성
- 컴포넌트 언마운트 시 이벤트 리스너 정리 필요성
✅ Verification successful
이벤트 핸들링 구현이 적절합니다.
naver.maps.Event.once를 사용하는 현재 구현은 다음과 같은 이유로 안전합니다:
once메서드는 이벤트를 한 번만 실행하고 자동으로 제거되므로 중복 등록 문제가 없습니다- 컴포넌트 언마운트 시 별도의 정리가 필요하지 않습니다
- NaverMap 컴포넌트에서 다른 이벤트 리스너들의 정리가 잘 구현되어 있습니다
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for event listener cleanup in the component
# Search for useEffect cleanup functions related to map events
rg -A 5 "useEffect.*map.*Event"
Length of output: 33
Script:
#!/bin/bash
# Let's check the component implementation more broadly
rg -A 10 "naver\.maps\.Event"
# Also check for any cleanup patterns in the file
rg -A 10 "useEffect.*map" src/utils/naver/addMarkerToMap.ts
# Check the entire file content to understand the implementation
cat src/utils/naver/addMarkerToMap.ts
Length of output: 4317
src/components/features/NaverMap/index.tsx (2)
61-63: 마커 초기화 로직이 개선되었습니다.
마커를 추가하기 전에 기존 마커를 초기화하는 로직이 useEffect 훅의 시작 부분으로 이동된 것이 적절해 보입니다. 이는 새로운 상태를 기반으로 로직을 실행하기 전에 기존 마커들을 정리하는 일관된 방식을 제공합니다.
65-67: 현재 위치 마커 표시 조건이 개선되었습니다.
markers.length === 0 조건을 추가하여 다른 마커가 없을 때만 현재 위치 마커를 표시하도록 수정된 것이 적절해 보입니다.
src/pages/MapView.tsx (3)
9-9: 타입 안정성 개선이 확인되었습니다!
BottomSheetType 임포트 추가로 타입 안정성이 향상되었습니다.
23-33: 상태 관리 구조가 잘 구성되어 있습니다!
훅들의 구성이 명확하고 의존성이 잘 관리되어 있습니다.
100-103: 상태 초기화 및 업데이트 로직이 적절합니다!
현재 위치 초기화 후 상태를 업데이트하는 순서가 올바르게 구현되어 있습니다.
| clearMarkers(); | ||
| setState({ data: null, type: null }, token); | ||
| navigator.geolocation.getCurrentPosition((position) => { | ||
| const { latitude, longitude } = position.coords; | ||
| updateLocation(latitude, longitude, 18); | ||
| }); | ||
| }, [clearMarkers, updateLocation]); | ||
| }, [clearMarkers, setState, token, updateLocation]); |
There was a problem hiding this comment.
위치 정보 오류 처리를 추가해주세요.
현재 위치 조회 시 발생할 수 있는 오류 상황에 대한 처리가 필요합니다.
const handleCurrentLocation = useCallback(() => {
clearMarkers();
setState({ data: null, type: null }, token);
navigator.geolocation.getCurrentPosition(
(position) => {
const { latitude, longitude } = position.coords;
updateLocation(latitude, longitude, 18);
},
+ (error) => {
+ console.error('위치 정보를 가져올 수 없습니다:', error);
+ // 사용자에게 오류 알림 표시
+ },
+ { timeout: 10000 }
);
}, [clearMarkers, setState, token, updateLocation]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clearMarkers(); | |
| setState({ data: null, type: null }, token); | |
| navigator.geolocation.getCurrentPosition((position) => { | |
| const { latitude, longitude } = position.coords; | |
| updateLocation(latitude, longitude, 18); | |
| }); | |
| }, [clearMarkers, updateLocation]); | |
| }, [clearMarkers, setState, token, updateLocation]); | |
| clearMarkers(); | |
| setState({ data: null, type: null }, token); | |
| navigator.geolocation.getCurrentPosition( | |
| (position) => { | |
| const { latitude, longitude } = position.coords; | |
| updateLocation(latitude, longitude, 18); | |
| }, | |
| (error) => { | |
| console.error('위치 정보를 가져올 수 없습니다:', error); | |
| // 사용자에게 오류 알림 표시 | |
| }, | |
| { timeout: 10000 } | |
| ); | |
| }, [clearMarkers, setState, token, updateLocation]); |
관련 이슈
close #78
📑 작업 내용
panTo,naver.map.Event를 통해 애니메이션을 추가했습니다.addMarkerToMap함수에서 실행하던clearMarks함수를useEffect상단에 위치시켜 모든 렌더링 사이클에서 기존 마커를 정리한 후, 새로운 상태에 따라 필요한 로직만 실행하도록 변경하였습니다.2024-11-28.3.12.05.mov
💬 리뷰 중점 사항/기타 참고 사항
zoom을 조정을 진행하게 되는데 이때 매끄럽지 못한 것이 아쉬운 것 같아요.Summary by CodeRabbit
BottomSheetType추가로 데이터 처리 방식 명확화.