Skip to content
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.

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

fix(client): 맵과 관련된 로직을 모두 마이그레이션 한 뒤 작동하게 합니다. #56

Merged
merged 28 commits into from
Jan 22, 2023

Conversation

po4tion
Copy link
Collaborator

@po4tion po4tion commented Jan 21, 2023

Describe your changes

  • 모바일에서 지도가 몇 번 움직이면 뻗어버리는 현상으로 지도 전체 레이아웃 마이그레이션
  • 지도 컴포넌트 최적화 및 지도 recoil 삭제
  • api 변경에 따른 history 타입 수정

Issue ticket number and link (optional)

Checklist

  • 🤔 이 프로젝트의 스타일 가이드를 따르나요?
  • 🤔 머지하기 전에 스스로 코드에 문제가 없음을 확인했나요?
  • 🤔 필요한 주석을 필요한 곳에 넣어주었나요?
  • ⛔️ (필수) base 브랜치를 pull 받았나요?!

Next Step Todo (optional)

Questions

  • 💬 질문 사항이에요!
  • 🤷‍♂️ 확인 받고 싶은 부분이에요!
  • 🔥 이건 꼭 확인해주세요!

image

지도에서 보이는 모든 식당 리스트로 보여주는 부분 타이틀은 해창님께 문의하였습니다!

history 부분에서 인피니티 스크롤 관련 부분 텍스트는 해당 브랜치에서 작업하지 않았습니다. 추후 인피니티 스크롤 기능과 함께 작업하겠습니다.

@po4tion po4tion added bug 예상하고 있던, 예상하지 못했던 기능의 오류 duplicate 최소 1번이라도 merge or closed 되었던 pr/issue fix bug를 고쳤을 경우 feat 새로운 기능을 구현하였을 경우 refactoring 기존의 코드를 리팩토링 하였을 경우 labels Jan 21, 2023
@po4tion po4tion requested a review from okinawaa January 21, 2023 21:16
@po4tion po4tion self-assigned this Jan 21, 2023
@okinawaa okinawaa changed the title Fix migration map fix(client): 맵과 관련된 로직을 모두 마이그레이션 한 뒤 작동하게 합니다. Jan 21, 2023
@okinawaa
Copy link
Contributor

viewList 화면에서 오류부분이있습니다.

image

@okinawaa
Copy link
Contributor

아무것도 고르지 않고 viewList로 들어왔는데 뱃지에 nationality라고 나와있는게 맞을까요? 기획간 소통 부탁드리겠습니다~!
image

@po4tion
Copy link
Collaborator Author

po4tion commented Jan 22, 2023

맵 전체 리스트를 view list로 가져올 때는 당연하게도 filterLikeCount가 존재할 수가 없는데 제가 이 부분을 간과했습니다. 수정완료하였습니다!
fix: 맵 전체 식당 리스트에서는 filterLikeCount 값이 필요가 없음

Copy link
Contributor

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

cool... so cool UI UX ..

@@ -20,21 +14,13 @@ import { motion } from "framer-motion";
import { useRouter } from "next/router";
import { Text } from "~/components/Common/Typo";
import { defaultScaleChangeVariants, framerMocker } from "~/constants";
import { useFetchDefaultMap } from "~/server/recent";
import { useResetRecentRecoilState } from "~/hooks";
Copy link
Contributor

Choose a reason for hiding this comment

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

이 훅자체는 common hook이라기 보다는, map이라는 도메인에 강력하게 연관되어있는것 같아요.

hooks/map 이라는 폴더를 만들고 그곳에 map에서만 사용하는 훅을 모아두는것은 어떤가요?
현재 이런식으로 도메인을 분리하지않는다면, hooks폴더가 무분별하게 커질것 같습니다!😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: hooks 폴더의 무분별한 확장을 방지하기 위한 도메인 분리

맞습니다! 감사합니다 수정 완료하였습니다!

import { useMap } from "~/contexts";
import { useCurrentLocationSetter } from "~/recoil/atoms";

const getCurrentLocation = (callback: (coord: [number, number]) => void) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

함수가 호출되고, 현재 위치를 받아올때까지 평균 2초정도 시간이 소요되는것 같은데,
카카오 서버로부터 로딩중인 상태를 받아올 수 있나요? 로딩처리가 필요할것만 같아요!
혹은 위치 받아옴 완료! 라는 결과값만 카카오로부터 받을 수 있다면

함수 호출 => 로딩중 => 카카오가 위치줌

이 사이에 로딩 indicator를 보여줌으로써 UX를 향상할 수 있을 것 같아요

Copy link
Contributor

Choose a reason for hiding this comment

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

global 로 isLoading state만든뒤,

successCallback과 failCallback에서 isLoading을 바꿔주면되겠네요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

2023-01-22.23.22.46.mov

fix: 현재 내 위치 버튼 클릭시 Lottie rocket loading process 적용

수정 완료하였습니다

Comment on lines 14 to 19
const executeReset = () => {
currentSelectCategoryReset();
currentHabitTitleReset();
currentSelectKeywordReset();
searchByFilteringReset();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

이거 useResetRecentRecoilState.ts 파일 자체에서 이렇게 함수를 만들어서 내보내는것 너무 좋은 것 같아요. executeReset이라고만 함수이름을 지으면 바깥부분에서 사용할때
리셋을 실행시킨다?.. 라고만 이해할거같은데
여러 값들을 한꺼번에 초기화 하는것이 이 함수의 목적중하나이므로
executeAllReset ?.. 정도로바꾸는것은 어때용?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: resetRecoil export Function naming change

수정 완료하였습니다!

setCurrentSelectKeyword,
uploadRecentHistoryMutate
]
[push, uploadRecentHistoryMutate]
);

const ExitClickHandler = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

대문자로 시작한 이유가 있나요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: 클릭 이벤트 함수의 네이밍 시작 대문자에서 소문자로 변경

아무 이유가 없습니다. 제 실수입니다. 수정 완료하였습니다!

@@ -74,11 +80,12 @@ const ViewListPage = () => {
overflow-y: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

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

이 위에 height 500px 에서
500은 어디서 나온 수치인가요?
식당이 잘려보여서요!


export const useMap = () => useContext(Context);

export const useKakaoDebounce = <T,>(value: T, delay?: number): T => {
Copy link
Contributor

Choose a reason for hiding this comment

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

제네릭의 T 다음에 comma는 무슨의미인지 궁금합니다!!

Copy link
Collaborator Author

@po4tion po4tion Jan 22, 2023

Choose a reason for hiding this comment

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

현재 파일의 확장자는 tsx입니다. JSX 문법 사용이 가능한 확장자이죠. <T> 라고 사용하게 되면 해당 파일은 <T>를 JSX태그로 인식하여 </T>를 사용하라고 에러를 내뱉습니다. 해결하기 가장 쉬운 방법은 첫째, .ts 확장자를 사용하면 됩니다. 두번째 방법은 <T>가 JSX 태그가 아니라는 것으로 우회하는 방식을 사용합니다. <T, > 문법을 사용하여 다른 인자를 보유할 수 있으니 해당 문법이 JSX태그 문법이 아니라는 것을 React에게 알려줍니다.

저는 확장자 변경으로 수정하겠습니다. 해당 파일을 살펴보니 굳이 tsx로 가져갈 필요가 없네요!

fix: hooks 파일 확장자 변경 tsx -> ts

참고 문서
microsoft/TypeScript#15713

Copy link
Contributor

Choose a reason for hiding this comment

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

holy.. thx bro

mapRef: RefObject<HTMLDivElement>;
});
type Props = {
center: { latitude: number; longitude: number };
Copy link
Contributor

Choose a reason for hiding this comment

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

index.d.ts에 선언된 Coordinate를 이용할 수 있을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: 이미 존재하는 coordinate 타입 재사용하기

감사합니다! 수정 완료하였습니다!

const [map, setMap] = useState<kakao.maps.Map | null>(null);
const mapRef = useRef<HTMLDivElement>(null);

// eslint-disable-next-line @typescript-eslint/no-empty-function
Copy link
Contributor

Choose a reason for hiding this comment

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

이 주석도 맨 위로 올리는것은 어떤가요?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: 컴포넌트에 존재하는 주석 최상단으로 올리기

수정 완료하였습니다! 예전에 제가 먼저 말해서 이렇게 하기로 했던 것 같은데 까먹었네요 히히....

useFetchRestaurantByEatingHabit
} from "~/server/recent";

const useMapHooks = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

useMap 이 아닌 useMapHooks라고 워딩하신 이유가 있나용?!
컨벤션은 Hooks를 따로 붙히지 않았어서요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: 중복되는 useMap hook 네이밍 변경

Context API를 사용하는 파일에서 이미 useMap이라는 네이밍을 사용하고 있어서 변경을 하였었습니다. 기존에 사용하던 useMap을 useKakaoMap으로 바꾸고 현 useMapHooks를 useMap이라고 수정하였습니다!

Comment on lines 51 to 53
useSetNavigation({
bottom: true
});
Copy link
Contributor

Choose a reason for hiding this comment

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

이거는 비즈니스 로직과 관련없으므로
파일의 맨위에 두는것은 어떤가요
useTheme이나
useSetNavigation은 항상 저는 첫줄에 넣긴합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

진짜 소름인건 파일 경로입니다. ㄷㄷㄷㄷㄷ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -20,6 +20,8 @@ import useViewList from "./viewList.hooks";
const ViewListPage = () => {
const app = useViewList();

console.log(app.searchByFiltering);
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log 필요한가용!?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines 14 to 29
type Props = {
center?: Common.Coordinate;
level?: number;
minLevel?: number;
maxLevel?: number;
draggable?: boolean;
zoomable?: boolean;
onClick?: (map: kakao.maps.Map, e: kakao.maps.event.MouseEvent) => void;
onDragStart?: (map: kakao.maps.Map, e: kakao.maps.event.MouseEvent) => void;
onDragEnd?: (map: kakao.maps.Map, e: kakao.maps.event.MouseEvent) => void;
onZoomChanged?: (map: kakao.maps.Map) => void;
onLoaded?: (map: kakao.maps.Map) => void;
onBoundChange?: (map: kakao.maps.Map) => void;
style?: CSSProperties;
children?: ReactNode;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

이것은 왜 interface 가 아닌, Type으로 선언하셨는지 궁금해요!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: component props type 선언 type -> interface로 수정

마이그레이션 과정에서 놓친 부분입니다! 감사합니다!

children: ReactNode;
}

const MapMarker = ({ position, children }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

PropsWithChildren을 쓰면 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: Component props type 적용

수정 완료하였습니다!

@okinawaa
Copy link
Contributor

나의 위치 누르고 하고 viewList하면 이상한곳의 음식점들이뜹니다, 정작 내 view에는 음식점 하나도 없습니다! 카카오톡으로 동영상 첨부했습니다~!

@okinawaa
Copy link
Contributor

image

이거 빈 roundedTag아직 해결 안된것 같습니다!

@po4tion
Copy link
Collaborator Author

po4tion commented Jan 22, 2023

fix: 내 위치 변경 시 map event 작동안하는 이슈 해결

위치 변경하고 view list 이상하던 문제 해결하였습니다!

@okinawaa okinawaa merged commit 620dcdc into main Jan 22, 2023
@okinawaa
Copy link
Contributor

@po4tion thx for your hard works ! You made it

okinawaa added a commit to okinawaa/toppings-client that referenced this pull request Jan 22, 2023
* feat: Recent type update

* fix: history 디자인 수정 적용 & 변경된 histories 타입 적용

* feat: 맵 코드 분리, lottie 적용

* fix: 지도 중앙 값 처리 로직 적용

* feat: 맵 hooks 파일 분리, 불필요한 컴포넌트, recoil 파일 제거, 지도에 그려지는 lottie 위치 조절

* fix: 맵 전체 식당 리스트에서는 filterLikeCount 값이 필요가 없음

* fix: hooks 폴더의 무분별한 확장을 방지하기 위한 도메인 분리

* fix: 더는 사용되지 않는 hook 제거

* fix: resetRecoil export Function naming change

* fix: 클릭 이벤트 함수의 네이밍 시작 대문자에서 소문자로 변경

* fix: 이미 선언하였던 타입 재사용하기

* fix: useMemo 한줄 선언 시 return 제거

* fix: hooks 파일 확장자 변경 tsx -> ts

* fix: VoidFunction 타입 사용하기:

* fix: 이미 존재하는 coordinate 타입 재사용하기

* fix: 컴포넌트에 존재하는 주석 최상단으로 올리기

* fix: 중복되는 useMap hook 네이밍 변경

* fix: useSetNavaigation 함수 내에서 선언시ㅣ 최상단으로 올리기:

* fix: Stack Vertical 사용으로 스타일 문법 축소 및 ul-li 태그 사용

* fix: ul tag -> ol tag

* fix: 이미 존재하는 페이지네이션 타입 확장하여 사용

* fix: 사용되지 않는 ref 제거:

* fix: 처음을 제외하고는 처음 위치 고정하여 그 다음에 눌렀을 때 바로 지도 center가 변경되도록 변경

* fix: 현재 내 위치 버튼 클릭시 Lottie rocket loading process 적용

* fix: component props type 선언 type -> interface로 수정

* fix: Component props type 적용

* fix: 내 위치 변경 시 map event 작동안하는 이슈 해결

* fix: 지도에서 전체 식당 viewList 라운드 뱃지 제거
@okinawaa okinawaa deleted the fix-migration_map branch February 8, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 예상하고 있던, 예상하지 못했던 기능의 오류 duplicate 최소 1번이라도 merge or closed 되었던 pr/issue feat 새로운 기능을 구현하였을 경우 fix bug를 고쳤을 경우 refactoring 기존의 코드를 리팩토링 하였을 경우
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants