-
Notifications
You must be signed in to change notification settings - Fork 0
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
[48기 박동철] 로그인, 회원가입 기능 구현 (백엔드 API서버와 통신 및 DB 저장 확인.) #4
base: main
Are you sure you want to change the base?
Conversation
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이 어떤 상황인지 알려주세요!
<Route path="/" element={<Login />} /> | ||
<Route path="/login" element={<Login />} /> |
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.
아 이건 그냥 npm start 시에 추가적으로 URL 타이핑하기 귀찮아서 혼자 편의상 바꿔놨던 건데 그대로 올려버렸네요...
src/pages/Join/Join.scss
Outdated
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.
해당 파일 하나로 Join 컴포넌트 내의 하위 컴포넌트들에 모두 적용시키고 계신데,
유지보수를 위해서 컴포넌트 하나 당 하나의 scss 파일을 생성해 적용하는 방식으로 수정해 주세요!
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.
그리고 해당 scss 파일에서 tag selector를 많이 사용하셨는데, 당장은 확정된 레이아웃일지라도 추후 변경되었을 때 내가 예상하지 못한 속성이 적용될 수도 있습니다.
tag selector는 정말 변경이 없다고 확신할 수 있는 상황 혹은 공용 스타일 파일인 reset, common.scss 등에서만 활용해 주세요!
src/pages/Join/Birthday.jsx
Outdated
<div> | ||
<select | ||
value={selectedYear} | ||
onChange={e => setSelectedYear(e.target.value)} | ||
> | ||
{years.map(year => ( | ||
<option key={year} value={year}> | ||
{year}년 | ||
</option> | ||
))} | ||
</select> | ||
</div> | ||
<div> | ||
<select |
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.
select 섹션마다 빈 div 태그를 감싸주셨는데, select 태그에 스타일 속성을 부여해서는 구현이 불가능한 부분인가요?
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.
일단 각 구간을 나눠야 한다는 생각에 이처럼 작성했는데 지금보니 쓸 데 없이 코드가 길어진 것 같기도 하네요.
참고하여 수정해보도록 하겠습니다!
src/pages/Join/JoinDone.jsx
Outdated
<p>이제 로그인해주세요.</p> | ||
</div> | ||
</div> | ||
<LoginButton text={'확인'} /> |
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.
<LoginButton text={'확인'} /> | |
<LoginButton text='확인' /> |
src/pages/Join/Join.scss
Outdated
p { | ||
&:first-child { | ||
font-size: 16px; | ||
} | ||
&:last-child { | ||
color: $grey80; | ||
font-size: 12px; | ||
} | ||
} |
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.
first-child 및 last-child를 활용해서 구현하는 것은 좋지만, 추후 유지보수가 필요할 때 오히려 불편함을 겪을 수 있습니다.
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.
반영하여 수정했습니다!
src/pages/Join/JoinInfo.jsx
Outdated
navigate('/join-done'); | ||
fetch('data/userData.json', { | ||
method: 'POST', | ||
headers: { | ||
'Content-Type': 'application/json;charset=utf-8', | ||
}, | ||
body: JSON.stringify(joinUserInfoData), | ||
}).then(); | ||
} else if ( |
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.
실제로 API 연결하실 때 분기처리 하시겠지만, navigate는 결과에 따라 처리되어야 합니다!
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 분기처리하도록 하겠습니다!
setJoinUserInfo(prev => { | ||
return { | ||
...prev, | ||
birthday: formattedBirthday, | ||
}; | ||
}); |
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.
개인의 차이일 수 있지만, 저같은 경우는 return문은 앞에 로직이 없으면 사용하지 않습니다. 동철님도 앞으로 개발하시면서 이처럼 동철님 나름의 기준을 세워보세요!
참고만 해주세요!
setJoinUserInfo(prev => { | |
return { | |
...prev, | |
birthday: formattedBirthday, | |
}; | |
}); | |
setJoinUserInfo(prev => ({ | |
...prev, | |
birthday: formattedBirthday, | |
})); |
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.
저도 평소엔 적지 않는 것을 선호하는 편인데 아무 생각 없이 작성한 거 같습니다...
src/pages/Join/JoinInfo.jsx
Outdated
<select> | ||
<option value="010">010</option> | ||
<option value="011">011</option> | ||
<option value="016">016</option> | ||
<option value="017">017</option> | ||
<option value="018">018</option> | ||
<option value="019">019</option> | ||
</select> |
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.
해당 파일 컴포넌트 밖에 국번을 의미하는 "AREA_CODE" 라는 이름의 상수데이터로 분리 후 map함수로 반복하도록 수정했습니다!
src/pages/Login/Login.jsx
Outdated
const handleLogin = () => { | ||
fetch('data/userData.json') | ||
.then(res => res.json()) | ||
.then(userData => { | ||
const userMatched = userData.find( | ||
user => | ||
user.email === loginUserInfo.email && | ||
user.password === loginUserInfo.password, | ||
); | ||
|
||
if (userMatched) { | ||
navigate('/join-done'); | ||
} else { | ||
alert('이메일 또는 비밀번호가 일치하지 않습니다.'); | ||
} |
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.
userData와 비교한다라는 건 해당 요청의 response로 모든 유저의 데이터가 들어오는 것을 상정하고 작성하신 것 같은데, 실제로는 request에 담긴 user의 정보가 유효한지는 백엔드에서 체크해서 유효하다/아니다에 대한 response만 전달받을 것 같네요!
로그인&회원가입 실습에서 경험하신 대로 작업하시면 될 것 같습니다.
혹은 이 부분이 의도된 기획인가요?
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.
의도한 바는 아니지만 mock데이터의 크기 자체가 작다보니 당연스럽게 생각했던 부분인 것 같습니다ㅠ
아직 fetch에 대한 개념이 좀 약해서 일단 받아온 후 어떻게 처리해야 할 지에 대해서만 고민했던 것 같네요...
말씀주신 부분 참고하여 수정하고 아래에 댓글 다시 남기도록 하겠습니다!
border: 1px solid #e6e6e6; | ||
border-radius: 16px; | ||
background-color: white; |
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에도 리뷰 남겼는데, 팀 공통에 대한 수정 사항은 개별 PR마다 따로 적용시키는 게 아니라 해당 내용만 수정하는 PR을 올려서 머지한 후 적용시켜야 합니다.
팀원 중 한 분만 main에서 새로 branch 생성해서 작업 후 PR 올려주세요!
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. 본 PR이 우리 팀의 웹 서비스 제품성에 어떠한 기여를 하였고,
사용자에게 어떠한 기대효과를 전달하는지 작성해주세요.
2. 이 브랜치에서 어떤 내용을 개발했는지 큰 제목과 상세 내역을 적어주세요.
로그인 기능 추가
회원가입 기능 추가
3. 개발한 화면을 캡쳐해서 첨부 해 주세요.
4. 이 브랜치에서 개발하면서 느낀 개발 성장포인트를 적어주세요.