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

[홍진호] week6 #273

Conversation

jinho0941
Copy link
Collaborator

@jinho0941 jinho0941 commented Mar 27, 2024

멘토에게

  • part1에서 마지막에 받은 피드벡을 수용하여 적용하였습니다.

  • 전과 비교했을때 복잡성이 줄었고 더 직관적이게 된거 같습니다! 감사합니다 👍

  • handle submit부분은 ui-controller에 넣기에는 받는 파라미터가 너무많아서 따로 파일을 만들었습니다.

  • part1 기간동안 지도해주셔서 감사했고, 덕분에 많은 지식과 정보를 얻었습니다. 저또한 마찬가지로 part2 잘 부탁드리며 승훈님이 알려주시는 내용을 잘 따라가 보겠습니다. 감사합니다^^

  • 후기 : ts의 소중함이 느껴지는 part1 이였습니다.

@jinho0941 jinho0941 requested a review from 1005hoon March 27, 2024 15:18
@jinho0941 jinho0941 added the 순한맛🐑 마음이 많이 여립니다.. label Mar 27, 2024
@jinho0941 jinho0941 changed the title [홍진호] week6 피드벡 [홍진호] week6 Mar 27, 2024
Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

전반적으로 많이 좋아졌네요 ㅎㅎ 이제 고민해보면 좋을 부분은 에러핸들링을 어디서 하면 될지, 변수명은 무엇이 적합할 지, 그리고 폴더 구조와 프로젝트 구조를 어떻게 하면 좋을지 등을 고민하면 될 것 같아요!

파트1 함께해서 즐거웠고, 2에도 잘 부탁드립니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

base_url 상수값 하나만을 위해서 스크립트 파일 하나를 새로 파는건 되게 음 투 머치인듯 해요 ㅎㅎ
오히려 상수들만을 기록하는 constant.js 를 만들어 거기서 상수값을 관리하는 형태는 어떨런지요

Comment on lines +12 to 17
if (!response?.ok) {
return {
success: false,
message: '이메일 혹은 패스워드가 잘못되었습니다.',
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

실제 서버에서 반환하는 오류 메시지를 로깅하는 부분이 없구뇽!
이러면 디버깅할때 지옥이 펼쳐진답니다.

서버에서 반환하는 에러미시도 로깅하도록 구현해볼까요?

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 +18 to +21
const responseData = await response.json()
console.log(responseData)
localStorage.setItem('accessToken', responseData.data.accessToken)
return { success: true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

로그인의 결과는 sucess값이 아닌 로그인 한 사용자에 대한 토큰과 사용자 정보일 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그렇군요! 그럼 클라이언트에서 localStorege를 써서 token을 저장하는 로직을 넣어야 겠내요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

우리 멘토링때 이야기했던것처럼 너무 작은 단위의 행동행동별로 구분하기보단
역할과 책임을 다룬 도메인별로 로직을 나눠 처리하도록 하면 어떨까요?

handleSubmit 이라는 함수는 로그인 폼에서만 활용되죠? 그럼 이건 오히려 main.js에 속하는게 좋아보여요!
form query selector와 함꼐 위치시켜주세요!

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 +18 to +29
const isValidEmail = validateLoginEmail(email)
if (!isValidEmail.success) {
emailErrorEl.textContent = isValidEmail.message
return
}

const isValidPassword = validateLoginPassword(password)

if (!isValidPassword.success) {
passwordErrorEl.textContent = isValidPassword.message
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

굳이 showError와 같은 함수를 사용하지 않은 이유가 있으실까요?
만약 요구사항에서 단순 텍스트 오류만 보여줘도 되는거라면 무시해주셔도 좋습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀해주신 것 처럼 요구사항에 텍스트 오류만 보여줘도 되서 뺏어요 ㅎㅎ

return
}

const isLogin = await login({ email, password })
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

멘토링에서 이야기했던것처럼 어디서 에러 핸들링을 하면 좋을지 고민해봐도 좋겠어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 참고하겠습니다!

@1005hoon 1005hoon merged commit 78520af into codeit-bootcamp-frontend:part1-홍진호 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
순한맛🐑 마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants