-
Notifications
You must be signed in to change notification settings - Fork 8
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
[FE] 비회원 로그인 페이지와 약속 조회/수정 페이지를 연결 #105
Conversation
Test Results5 tests 5 ✅ 3s ⏱️ Results for commit 8f61b3a. ♻️ This comment has been updated with latest results. |
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.
PR 감사합니다 빙봉!
사소한 부분들에 대해서만 리뷰 남겼습니다 :)
반영 부탁드려요!
staleTime: 0, | ||
}); | ||
|
||
if (!meetingSchedules) return <></>; | ||
if (!('attendeeName' in meetingSchedules)) return <></>; | ||
console.log(meetingSchedules); |
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.
[P1]
여기 콘솔 붙어있네요!
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.
오호!! 콘솔 지우는거 깜빡했네요😅 감사합니다! 반영하였습니다
<TimePicker | ||
firstTime={firstTime} | ||
lastTime={lastTime} | ||
availableDates={availableDates} | ||
meetingSchedules={meetingSchedules} | ||
/> | ||
) : ( | ||
<></> |
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.
[P2]
Fragment로 return하게 되면 렌더링이 발생하는 것으로 알고 있습니다.
렌더링 하지 않으려면 return null로 해주어야할 것 같아요!
https://ko.react.dev/learn/conditional-rendering#conditionally-returning-nothing-with-null
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.
페이지 연결한다고 고생했습니다 빙봉!!
간단한 코멘트 몇개 남겼으니 수정 후 다시 요청 부탁드릴게요~
frontend/src/apis/getHeaders.ts
Outdated
export default function getHeaders(): Record<string, string> { | ||
const headers = { 'Content-type': 'application/json' }; | ||
const token = localStorage.getItem('momoToken'); | ||
const token = getCookie('token'); |
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.
[P3]
token 상수화가 필요할 것 같습니다!
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 { handleGetMeetingSchedules } from '@apis/getMeetingSchedules'; | ||
import { getMeetingMySchedule } from '@apis/getMeetingSchedules'; |
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.
[Comment]
오늘 약속 참여자 자신의 스케쥴을 조회하는 API 엔드포인트가 me
로 변경되었는데, getMeetingMySchedule함수 이름도 getMeetingMeSchedule로 변경하는 것이 좋을까요? 🤔
함수명 자체는 어색한 것 같은데 백엔드 엔드포이트와 맞추려면 어쩔수 없나 싶기도 하네요~
빙봉은 어떻게 생각하시나요
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.
@hwinkr
해리가 제안 주신 의견대로라면 API path가 변경되면 또 함수명을 그에 맞게 수정해야 하지 않을까요?
API 경로가 변경될 때마다 함수명도 수정해야 한다면, 프론트엔드 코드가 백엔드 API 구조에 너무 의존하게 되는 것 같습니다🤔
API 호출 함수명은 API 경로와 독립적으로, 해당 기능의 의미를 명확히 전달할 수 있도록 프론트엔드 관점에서 정의하는 것은 어떨까요?
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
로 하시죠!
const params = useParams<{ uuid: string }>(); | ||
const uuid = params.uuid!; |
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.
[P2]
현재 useParams
를 많은 컴포넌트들에서 호출하고 있고, 호출한 후의 역할이 서버에서 전달해주는 uuid를 찾는 역할이니 커스텀 훅 useUUID
를 만들어서 중복을 줄여보는 것은 어떨까요?
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.
staleTime: 0, | ||
}); | ||
|
||
if (!meetingSchedules) return <></>; | ||
if (!('attendeeName' in meetingSchedules)) return <></>; | ||
console.log(meetingSchedules); |
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 { handleGetMeetingSchedules } from '@apis/getMeetingSchedules'; | ||
import { getMeetingMySchedule } from '@apis/getMeetingSchedules'; |
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.
@hwinkr
해리가 제안 주신 의견대로라면 API path가 변경되면 또 함수명을 그에 맞게 수정해야 하지 않을까요?
API 경로가 변경될 때마다 함수명도 수정해야 한다면, 프론트엔드 코드가 백엔드 API 구조에 너무 의존하게 되는 것 같습니다🤔
API 호출 함수명은 API 경로와 독립적으로, 해당 기능의 의미를 명확히 전달할 수 있도록 프론트엔드 관점에서 정의하는 것은 어떨까요?
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.
리뷰 반영 감사해요 🙌
2eaf0cb
to
8f61b3a
Compare
관련 이슈
작업 내용
특이 사항
리뷰 요구사항 (선택)