-
Notifications
You must be signed in to change notification settings - Fork 25
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
[하신혜] Sprint 3 #45
base: basic-하신혜
Are you sure you want to change the base?
The head ref may contain hidden characters: "basic-\uD558\uC2E0\uD61C-sprint1"
[하신혜] Sprint 3 #45
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 요청 시 작업 결과물을 확인할 수 있는 url 을 첨부해 주시면 감사하겠습니다.
기존 url로 확인 시 배포가 되어있지 않은 것 같아 코드로만 리뷰 진행하였습니다
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.
파일 이름에 2가 붙은 이유가 있을까요? register.js 는 없는 것 같은데 2를 붙이신 이유가 궁금합니다
</p> | ||
</form> | ||
</div> | ||
<div id="modal" class="modal"> | ||
<div class="modal-content"> | ||
<p id="modalMessage" class="modalmessage">모달 내용</p> |
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.
클래스 이름은 하이픈 인데 id는 카멜 케이스인 이유가 있을까요? 그리고 id 를 사용하신 이유가 유일 요소로 사용하기 위해서가 맞을까요? css 를 적용을 위해서라면 범용으로 적용할 방법은 없는지 고민해 보시는 것도 좋을 것 같습니다
const emailError = document.getElementById('emailError'); | ||
const passwordError = document.getElementById('passwordError'); | ||
|
||
const emailPattern = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; |
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.
사소한 정보를 전달드리자면 정규 표현식은 정규 표현식이라 바로 알 수 있도록 regex 를 붙여주시면 좋습니다.
// 이메일 유효성 검사 | ||
if (emailValue === "") { | ||
emailInput.classList.add('error'); | ||
emailError.textContent = '이메일을 입력해주세요.'; | ||
emailError.style.visibility = 'visible'; | ||
isValid = false; | ||
} else if (!emailPattern.test(emailValue)) { | ||
emailInput.classList.add('error'); | ||
emailError.textContent = '잘못된 이메일 형식입니다.'; | ||
emailError.style.visibility = 'visible'; | ||
isValid = false; | ||
} else { | ||
emailInput.classList.remove('error'); | ||
emailError.style.visibility = 'hidden'; | ||
} | ||
|
||
// 비밀번호 유효성 검사 | ||
if (passwordValue === "") { | ||
passwordInput.classList.add('error'); | ||
passwordError.textContent = '비밀번호를 입력해주세요.'; | ||
passwordError.style.visibility = 'visible'; | ||
isValid = false; | ||
} else if (passwordValue.length < 8) { | ||
passwordInput.classList.add('error'); | ||
passwordError.textContent = '비밀번호를 8자 이상 입력해주세요.'; | ||
passwordError.style.visibility = 'visible'; | ||
isValid = false; | ||
} else { | ||
passwordInput.classList.remove('error'); | ||
passwordError.style.visibility = 'hidden'; | ||
} |
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.
보시면 이메일, 패스워드 정보를 가져와서 검사를 합니다.
이 때 validateForm 내부에서는 이메일인지, 비밀번호인지 구분하지 않고 한 번에 모두 검사를 진행하고 있습니다.
이는 예측하지 못한 결과를 불러올 수 있습니다. 예컨데 비밀번호 검수를 진행하려고 했는데 이메일 검사 로직을 같이 진행해서 이메일 관련한 오류 문구가 뜰 수 도 있습니다.
이는 공통 로직이 아니기 때문에 분리가 필요한 사안입니다. 개발의 컨셉과 디자인 패턴마다 다르지만 기본적으로 함수는 하나의 일만 하는 걸로 생각하고 코드를 작성해 보시는 연습을 해 보시면 좋을 것 같습니다.
아래는 참고 자료입니다.
const USER_DATA = [ | ||
{ email: '[email protected]', password: 'codeit101!' }, | ||
{ email: '[email protected]', password: 'codeit202!' }, | ||
{ email: '[email protected]', password: 'codeit303!' }, | ||
{ email: '[email protected]', password: 'codeit404!' }, | ||
{ email: '[email protected]', password: 'codeit505!' }, | ||
{ email: '[email protected]', password: 'codeit606!' } | ||
]; |
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.
다음에는 상수는 별도의 파일로 분리해서 사용해 보시는 걸 추천드립니다.
function openModal(message) { | ||
modalMessage.textContent = message; | ||
modal.style.display = 'block'; // 모달을 보이게 설정 | ||
} |
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.
모달의 경우 잘 구현하신것 같습니다. 최근에는 dialog 라는 html 태그가 있어 더욱 쉽게 모달을 구현할 수 있는 방법이 있으니 참고해 보시면 좋을 것 같습니다
}); | ||
|
||
// 이메일, 비밀번호, 비밀번호 확인 유효성 검사 및 회원가입 버튼 활성화 | ||
function validateForm() { |
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.
login.js 에 있는 validateForm 함수와 동일한 것으로 보입니다. 공통 로직을 분리해 볼 수 있을 것 같습니다.
기본 요구사항
공통
const USER_DATA = [
{ email: '[email protected]', password: "codeit101!" },
{ email: '[email protected]', password: "codeit202!" },
{ email: '[email protected]', password: "codeit303!" },
{ email: '[email protected]', password: "codeit404!" },
{ email: '[email protected]', password: "codeit505!" },
{ email: '[email protected]', password: "codeit606!" },
];
로그인 페이지
회원가입
심화 요구사항
공통
랜딩 페이지
로그인, 회원가입 페이지 공통
멘토에게
안녕하세요 존경하는 멘토님.. 심화 부분이 텅텅 빈 것을 보고 상심하셨을거같습니다.. 죄송합니다 😫
기본적으로 기본 요구사항에 시간 할애를 많이 했습니다.
저번 스프린트 1/2 과제때 멘션하셨던 부분 간단하게는 수정해보았는데 기존에 작업하던 내용인 만큼 이제와서 클래스 명 바꾸기가 쉽지않아서 이번에는 그대로 작업해보았습니다. 새로 작성한 클래스명은 조금 신경써보았습니다!
모달 버튼 관련해서 이해하지 못하고 중도에 포기했습니다 ㅠㅠ 미디어쿼리 관련해서는 관련 강의를 보긴하였으나 적용해보진 못하였습니다. 다음 주에 다 못한 심화과정 시간 날때마다 틈틈히 진행해 볼 예정입니다.
실업무에서도 이렇게 일일히 손으로 다 써야하나요 어흑... 이번 과제때 많이 힘들었습니다 ㅠㅠ