Issue169 마이페이지 북마크 된 음식점 지도 뷰 추가 #170
Conversation
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
| interface EventMapMarkerProps { | ||
| position: { lat: number; lng: number }; | ||
| isClicked: boolean; | ||
| onClick: () => void; |
There was a problem hiding this comment.
props들의 네이밍을 더 구체화 하는 것이 좋을 것 같아요! 물론 지금도 maker누르면 발생하는 여부인가보다 대충 어림짐작할 수 있어도 정확한 의미 전달은 힘들어서 네이밍에 수정이 필요해보입니다
There was a problem hiding this comment.
더불어서 맥락을 위해서 추가적인 설명이 필요한 prop이나 변수에는 주석을 달아주세요! 저희가 항상 만나서 이야기하고 회의할 수 있는게 아니다보니 코드 안에서의 컨텍스트도 중요합니다!
There was a problem hiding this comment.
그 부분에 대해서는 생각을 못 했네요! 코멘트로 설명 추가하고 props 명도 각각, markerPosition, isMarkerClicked, onMarkerClick로 변경해 봤습니다!
| interface EventMapMarkerProps { | ||
| position: { lat: number; lng: number }; | ||
| isClicked: boolean; | ||
| onClick: () => void; |
There was a problem hiding this comment.
더불어서 맥락을 위해서 추가적인 설명이 필요한 prop이나 변수에는 주석을 달아주세요! 저희가 항상 만나서 이야기하고 회의할 수 있는게 아니다보니 코드 안에서의 컨텍스트도 중요합니다!
| "react-query": "^3.39.1", | ||
| "react-router-dom": "^6.3.0", | ||
| "react-scripts": "^5.0.1", | ||
| "styled-components": "^5.3.5", | ||
| "swiper": "^9.4.1", |
There was a problem hiding this comment.
많은 슬라이더 라이브러리가 있는데 얘를 선택한 이유가 있을까요?
There was a problem hiding this comment.
처음에는 직접 구현해 보려다가 시간 안에 못할 것 같아서 라이브러리를 찾아보게 되었습니다! 그래서 react-slick이랑 swiper를 살펴봤는데, 사용 방법은 둘 다 거의 비슷하더라고요. 그런데 swiper는 slide 할 때는 click 이벤트를 막아 주는 반면, react-slick은 바로 못 해주고 그걸 하기 위한 과정이 조금 복잡했어요. 그리고 이건 제대로 테스트 한건 아니지만, Xcode에서 아이폰이랑, 아이패드에서 터치해서 슬라이드 할 때 swiper를 사용할 때 제대로 원하는 인터렉션을 할 수 있었고요. 찾아보니까 swiper가 mobile first에 초점을 맞춰서 그렇다고 하더라고요!
아무래도 맛집 서비스 자체는 모바일 타겟인 것 같아서 swiper를 선택했습니다!
| } | ||
|
|
||
| function EventMapMarker({ position, isClicked, onClick }: EventMapMarkerProps) { | ||
| const icon = isClicked ? ClickedPinIcon : PinIcon; |
There was a problem hiding this comment.
아이콘을 아예 갈아치우는 방법도 있지만 컬러만 바꾸도록 할 수도 있지 않을까요?
There was a problem hiding this comment.
아이콘을 아예 갈아치우지 않고, 컬러만 바꾼다는 게 정확히 뭘 말하는 건가요?? 조금 더 자세히 설명 부탁합니다!
There was a problem hiding this comment.
아 그냥 만약 기존 방식대로 색만 바뀌는 거라면 props로 컬러 받고 svg 컴포넌트에 적용한다는 이야기였어요!
src/components/pages/MypagePage/BookmarkMapPage/BookmarkMapPage.tsx
Outdated
Show resolved
Hide resolved
원래는 슬라이드 했을 때 해당 지도 위치로 이동하지 못하는 것이 맞습니다! 이 부분에 대해서는 생각을 못 해봤네요. 리팩토링하면서 한 번 추가해 봤는데, 어떤가요??
제목은 |
| markerPosition, // 마커를 표시할 위치 - 경도, 위도 | ||
| isMarkerClicked, // 마커가 클릭 여부 | ||
| onMarkerClick, // 마커가 클릭되었을 때 발생할 액션 | ||
| }: EventMapMarkerProps) { |
There was a problem hiding this comment.
앗 주석을 달아준 것은 너무 좋은데 함수의 인자가 아니라 인터페이스에 달아주는 것은 어떨까요?
그리고 일반 주석이 아니라 JSDoc 주석을 사용하면 사용처에서도 빠르게 확인할 수 있답니다!
현재
JSDOC 주석
There was a problem hiding this comment.
오 이런 게 있었군요!! 다음부터 이렇게 정리해 보겠습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
#169
* 음식점 정보 아이템에 북마크 저장 표시는 나중에 #167랑 머지한 후에 연동할 예정입니다.