refactor: 마커 렌더링을 최적화 한다 #737
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 겸사겸사 모바일에서는 디테일이 창이 바로 열리지 않도록 하는 기능도 구현 [#732]
|
🚀storybook: https://storybook.carffe.in/ |
1 similar comment
|
🚀storybook: https://storybook.carffe.in/ |
feb-dain
requested changes
Sep 15, 2023
|
|
||
| import type { StationMarker } from '@type'; | ||
|
|
||
| import CarFfeineMarker from '../../../ui/CarFfeineMarker/index'; |
| stationMarkers | ||
| ); | ||
|
|
||
| const remainMarkerInstances = getRemainMarkerInstances( |
Comment on lines
+29
to
+31
| const newMarkers = markers.filter((marker) => | ||
| prevMarkerInstances.every((prevMarker) => prevMarker.stationId !== marker.stationId) | ||
| ); |
There was a problem hiding this comment.
[질문] newMarkers가 이전 범위 바깥에 있었던 새로운 마커들을 의미하나요?
There was a problem hiding this comment.
새로 fetch한 마커들 에서 이전의 마커 영역에 겹치는 부분을 제외한 나머지 영역에 해당하는 마커들을 의미합니다
| }); | ||
| }; | ||
|
|
||
| const getRemainMarkerInstances = ( |
There was a problem hiding this comment.
getRemainedMarkerInstances 아닐까요??
Comment on lines
+30
to
+31
| const latitude = screen.get('isMobile') ? lat + latitudeDelta / 3 : lat; | ||
| const longitude = screen.get('isMobile') ? lng : lng - calculatedMapDelta / 2; |
There was a problem hiding this comment.
말 그대로 수치의 절반이랑 3분의 1이란 의미인데 이건 어떻게 상수화 할 수 있을까요?
There was a problem hiding this comment.
어떤 의미인지, 왜 사용했는지 코드상으로 알 수 없을 때 주석이나 상수를 활용하면 좋겠어요
There was a problem hiding this comment.
무조건 상수화를 생각하면 오히려 가독성을 해칠 수 있으니 앞으로 상수화 하기 어려운 요소에 대해서는 주석으로 설명을 추가해두겠습니다!
|
🚀storybook: https://storybook.carffe.in/ |
feb-dain
approved these changes
Sep 15, 2023
frontend/src/components/google-maps/marker/hooks/useRenderStationMarker.tsx
Show resolved
Hide resolved
|
🚀storybook: https://storybook.carffe.in/ |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📄 Summary
Marker 컴포넌트에서 마커를 렌더링하고, 이벤트 핸들러를 달아주던 기존 방식에서 StationMarkersContainer 컴포넌트에서 마커의 생성과 관련된 모든 로직을 일괄 처리 하는 방식으로 수정했습니다.
추가적으로 모바일에서 StationDetailsWindow가 마커 클릭시에 바로 열리지 않도록 수정했고, 모바일 환경에서 마커 클릭시 지도의 이동이 부자연스러웠던 것도 개선했습니다.
로직 변경으로 인해 기존에는 하위 컴포넌트(마커)의 개수만큼 마커 인스턴스 전역 상태 변경이 발생했던 문제를 개선했습니다. 그 효과로 드르륵 찍히던 마커들이 한번에 찍히게 되었습니다.
개선 전
개선 후
🕰️ Actual Time of Completion
🙋🏻 More
로직을 개선하면서 기존의 기능 중 동작하지 않는 기능이 생겼습니다. 이전 방식을 그대로 사용해서 기능을 살리려 했으나 이전 방식이 상태 변경에 의한 사이드 이펙트에 의존하는 방식이다보니 이를 개선할 필요성이 있다고 판단해 다음 이슈에서 해결하도록 하겠습니다.
동작하지 않는 기능
close #732