-
Notifications
You must be signed in to change notification settings - Fork 9
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
Issue166 마이페이지(나의 맛집, 나의 리뷰보기) 추가 #168
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>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
return ( | ||
<S.Container> | ||
<S.HeaderWrapper> | ||
<LeftIcon onClick={() => navigate(-1)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 이전 페이지로 가기 위해서 navigate(-1)을 사용하는 코드가 기존에도 여기저기에 있었군요..
개인적으로 사용자 입장에서 -1로 구현된 경우에 불편한 점이 꽤 많더라구요! 링크나 히스토리를 통해서 해당 페이지로 바로 들어왔을 때에는 히스토리 스택에 -1이 같은 서비스 내에 페이지가 아니기 때문에 어색하다고 느꼈어요. 기회가 되면 다음에 navigate(-1) 대신 실제 그 페이지의 상위 페이지로 이동하도록 구성해놓으면 좋겠네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
무슨 말인지 알 것 같아요! 하지만 전체적으로 바꾸는 것은 리팩터링할때 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 이거 나의 맛집 지도 구현하면서 애슐리랑 이야기해봤는데 상황에 따라서 -1 로 구현되는게 더 자연스러운 것이 있더라구요. 명확하게 상위페이지로 이동하는 경우에만 바꾸도록 전체적으로 merge되고 나서 리팩터링 해보겠습니다 (이것저것 짤려서 기능 구현이 되어있어서요!) #173 로 새로 이슈 작성해놨어요
{isError && error instanceof Error && ( | ||
<ErrorImage errorMessage={error.message} /> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 렌더부에서 error의 클래스까지 검사해주는 게 뭔가 버거운 느낌이 드는데요.. 이걸 미리 return 문이 아닌 로직 위쪽에서 해줄 수 있는 방법은 없을까요? 그리고 이 로직대로라면 error가 Error 클래스의 인스턴스가 아닌 경우에는 오류조차 보이지 않을 것 같은데 어색하다고 느껴집니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 서버에서 에러 메시지를 보내주고 있기 때문에 error.message를 사용하기 위해 이렇게 했습니다.또한 만약 바꾸게 된다면 해당 코드가 프로젝트 전반적으로 반복되고 있기 때문에 이거 하나만 바꾸는 것이 아닌 전체적으로 ErrorBoundary로 감싸는 방식으로 해야할 것 같습니다. 따라서 다른 이슈로 해결해보도록 하겠습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 해당 코드가 전반적으로 반복되고 있다면 더더욱 반환되는 Error가 꼭 특정 타입 형식이도록 강제하는 것도 좋을 것 같아요! throw되는 오류 그 자체가 아니고 useQuery에서 반환하는 값이기 때문에 특정 오류 형식으로 항상 반환되도록 할 수 있을 것 같은데 고민해보면 좋을 것 같아요!
import StoreList from "components/common/StoreList/StoreList"; | ||
import Text from "components/common/Text/Text"; | ||
|
||
function MypagePage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호.. MypagePage라는 네이밍은 쉽지 않네요 ㅋㅋ
분명 두 분이 함께 논의를 했다면 이 네이밍에 대해서 이야기 했을 것 같은데 어떻게 이 이름으로 결정이 됐나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 페이지에는 ~Page라고 네이밍 되어있더라구요. 그래서 저희는 프로젝트의 컨벤션을 유지하는 것이 가장 중요하다고 생각했습니다. 그렇다고 해당 페이지 이름을 마이페이지가 아닌 다른 것으로 바꾸는 것은 너무 억지스럽기도 했구요! 따라서 지금처럼 MypagePage라고 이름을 정하게 되었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 MyPage로 하면 안되는 이유는 무엇인가요? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
약간 저희가 생각한 이유는 (페이지이름)+Page 라서 Page는 컴포넌트를 구분하기 위해 붙이는거고 해당 페이지의 이름은 Page앞에 있는 부분이라고 생각했거든요! 그냥 해당 페이지 이름 자체를 폴더명으로 해도 되는거면 MyPage로 바꾸도록 하겠습니다ㅎㅎ
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
Co-authored-by: Rulu <hafnium1923@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새로운 기능인 마이페이지 구현 수고하셨습니다 👍👍
블링이 남긴 부분들 제외하고 코드리뷰 했을 때 제가 남긴 리뷰는 모두 마이너해서 바로 approve로 코드리뷰 남깁니다
} | ||
); | ||
|
||
const bookmarkedStoreData = data ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data를 따로 변수에 할당해서 사용하지 않아도 되지 않을까요? 혹시 이렇게 한 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data가 undefined 형식으로 들어올 수 있어서 한번 정재해줘야 각종 버그가 일어나지 않았습니다.
onSuccess={() => { | ||
queryClient.invalidateQueries([ | ||
"reviewDetailStore", | ||
{ restaurantId: restaurant.id }, | ||
]); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 함수도 위의 선언된 다른 함수들 처럼 (handle~) 따로 분리해서 선언하는 것이 더 좋을 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 적용하겠습니다.
import StoreList from "components/common/StoreList/StoreList"; | ||
import Text from "components/common/Text/Text"; | ||
|
||
function MypagePage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 MyPage로 하면 안되는 이유는 무엇인가요? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마이페이지 > 맛집 전체보기 눌렀을 때 '나의 맛집' 오른쪽에 '지도' 텍스트는 무엇인가요?
마이페이지에서 로그아웃했을 때 해당 페이지에 그대로 있습니다.
로그인하지 않고 마이페이지로 진입하면 '다시 로그인해 주세요'가 세번 정도 발생합니다!
나의 리뷰(/my-page/reviews) 페이지 또는 마이페이지에서 리뷰 수정/삭제 버튼을 누르면 가게 상세페이지로 이동합니다!
src/types/common/bookmarkTypes.ts
Outdated
@@ -0,0 +1,10 @@ | |||
export interface BookmarkStore { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bookmarkstore 타입이랑 store 타입이랑 큰 차이가 없어서 Pick이나 Omit을 사용하는 것도 괜찮아보여요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omit을 사용해보겠습니다.
|
||
export interface UserReview extends ReviewInputShape { | ||
id: number; | ||
restaurant: Pick<Store, "id" | "name" | "imageUrl">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BookmarkStore가 아니라 Store에서 가져온 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 더 큰 범위로 생각했습니다. 유저가 리뷰한 가게가 꼭 북마크 가게란 법은 없으니까요!
마이페이지 > 맛집 전체보기 눌렀을 때 '나의 맛집' 오른쪽에 '지도' 텍스트는 무엇인가요?
마이페이지에서 로그아웃했을 때 해당 페이지에 그대로 있습니다.
로그인하지 않고 마이페이지로 진입하면 '다시 로그인해 주세요'가 세번 정도 발생합니다!
나의 리뷰(/my-page/reviews) 페이지 또는 마이페이지에서 리뷰 수정/삭제 버튼을 누르면 가게 상세페이지로 이동합니다!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 로그아웃이 어떤 경로든 하나로 작동되게 되어있습니다. 마이페이지에서 로그아웃 했을 때 메인 화면으로 돌아가게 하려면 전체적용으로 하는 방법밖에 모르겠는데 그렇게 해도 되나요?
네! 그렇게 하고 싶으면 그렇게 하면 됩니다ㅎㅎ 팀원과 상의 후 정하면 될 것 같아요! 컨텍스트로 가지고 있는 로그인 상태를 사용해서 리액트 라우터로 로그인 상태에 따라 이동시켜도 되겠죠!
직접 경로를 치고 들어갔을 때를 말씀하시는 것 같습니다. 확인해봤는데 fetchUserProfile -> fetchBookmarkList -> fetchUserReviewList 순서로 accessToken 확인 부분이 작동하는 것같습니다. 근데 어떻게 막아야 할 지 모르겠어요. accessToken 작동 방식도 잘 이해가 안되고 기존에 에러 처리 어떻게 진행된건지도 감만 잡았지 제대로 이해하고 있는게 아니라서 혹시 좀 힌트 주실 수 있을까요?
음 이건 예상되는 문제가 여러 갠데,
-
일단 리팩터링을 좀 하면 해결될 거에요! 지금 api 각각 axios에서 에러 처리 + 리액트 쿼리로도 에러 처리하면서 에러를 다루는 부분이 많이 분산되어 있는데 에러바운더리를 구현하든 뭐하든 해서 에러처리로직을 통일하면 어느정도 해결될 것 같아요!
-
첫번째 질문의 답처럼 리액트라우터로 로그인 여부에 따라 특정 페이지가 렌더링 되도록 정할 수도 있어요! 경로로 접근할 때 로그인 조건에 걸리면 다른 페이지를 렌더링해서 아예 fetch가 작동하지 않도록..!
+ fetch-fetch-fetch 하면서 alert 여러 번 뜨는 거는 아마 세개가 다 react query 키값이 달라서 각각의 onError가 따로 동작하는 거 같습니다
+ 지금 로그인 동작을 간단히 설명하면,
메뉴의 로그인 버튼 클릭 -> 로그인 페이지로 이동(/login) -> 로그인 요청 -> 깃헙과 백엔드에서 처리 -> accessToken 응답 -> accessToken을 세션스토리지에 저장 및 컨텍스트 isLoggedIn 상태를 true로 설정
입니다!
요 문제는 당장 안 고치셔도 됩니다! 지금까지 프로젝트에는 정말 몇몇 페이지, 기능만 제외하면 로그인 여부가 상관 없었어서 해당 기능에서 바로 로그인에 따른 에러 처리를 해서 통일된 규정이 없었습니다. 이제부턴 로그인 여부가 중요한 마이페이지 같은 기능이 많이 생겼으니까 각각 기능에 따로 에러 처리하는 방식 말고 통일된 방식으로 어떻게 구현할 지 생각하면 좋을 것 같습니다 :D
자세한 설명 감사합니다! 에러 바운더리는 이슈도 새로 작성해놔서 전체적으로 다시 리팩터링 해보면 될 것 같습니다. 말씀드린 것처럼 로그아웃을 하면 홈으로 돌아오도록 수정했습니다.
이거는 적용하고 싶어서 찾아보고 있긴한데 아직은 잘 모르겠어서 조금 더 공부하고 나중에 적용해보고 싶습니다. 정보 찾는것도 어렵네요ㅎㅎ.. 리뷰 항상 감사드립니다. 많이 성장하고 있는 것 같아요!! |
SonarCloud Quality Gate failed. 0 Bugs 0.0% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
#166
전체보기
를 클릭해서 전체 목록을 볼 수 있다.전체보기
를 클릭해서 전체 리뷰 작성 목록을 볼 수 있다.