[김태일] Sprint4 #161
[김태일] Sprint4 #161
Conversation
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
태일님 4번째 미션 제출 고생하셨습니다!
JS 가 처음이시라 쉽지 않으셨을텐데 잘 작성하셨어요.
이렇게 작업하셨다면 작업하지 못했다고 표시하신 사항들도 금방 구현하실 수 있으실테니
나중에 구현해보시면 좋겠습니다!
다음 React 미션도 화이팅입니다.
- 디자인상 에러가 나고 input border color가 red가 적용되어야 하는데 이것이 구현되지 않은 것 같습니다. 확인해보시고 추가하시면 좋을 것 같아요.
- 비밀번호 토글 버튼인 .password-icon의 경우 부모태그인 .password-input을 기반으로 상대값으로 위치가 정해져서 에러 메시지가 나오면 내려가게 됩니다. 비밀번호 인풋과 확인 인풋 모두 에러 메시지가 나와도 토글 버튼의 위치는 고정되도록 수정해보세요!
- 회원가입 페이지에서 비밀번호 입력 후 비밀번호 확인 입력하고 다시 비밀번호를 변경하면 비밀번호 확인 인풋은 재확인 로직을 실행하지 않네요~ 한번 다양한 케이스를 테스트해보세요.
|
|
||
| </div> | ||
|
|
||
| <script src="./sign-in.js"></script> |
There was a problem hiding this comment.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
| <input type="email" id="email" name="email" placeholder="이메일을 입력해주세요."> | ||
| <input type="email" id="email" name="email" placeholder="이메일을 입력해주세요." | ||
| class="error-input"> | ||
| <p id="emailErr" class="errMsg">잘못된 이메일 형식입니다</p> |
There was a problem hiding this comment.
💊 제안
미리 에러 메시지가 나타날 태그를 만들어 주셔서 DOM 조작을 최소화하신 점은 좋습니다!
다만 에러 메시지는 에러 상황에 따라 추가되도록 하는 것이 더 명확할 것 같아요!
지금처럼 미리 메시지가 들어가 있는 경우는 에러 나는 경우가 하나인 경우에 적절할 것 같습니다.
There was a problem hiding this comment.
💊 제안
로그인에서 사용하는 js 파일과 에러메시지나 로직 측면에서 동일한 것이 많습니다~
이런 경우 중복을 줄이기 위해 반복 사용되는 함수, 정규식, 에러메시지들은 다른 파일로 분리하시고 각 js 파일에서 가져와서 사용하시는 것을 추천드려요!
| function emailValidation(email) { | ||
| const emailRegex = /^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$/; | ||
| return emailRegex.test(email); | ||
| } |
There was a problem hiding this comment.
💊 제안
sign-up.js 파일에서도 동일한 정규식을 사용하고 있는데 그때마다 선언해서 쓰면 문제가 있을 것 같아요.
정규식을 반복해서 선언하면 유지보수가 어려워지고, 코드의 일관성이 떨어질 수 있습니다. 만약 정규식을 수정해야 할 때 여러 곳에서 변경해야 하고 실수로 서로 다른 정규식을 사용할 위험도 커집니다.
이렇게 두개의 파일에서 사용하고 있으니 해당 정규식과 test 함수를 따른 파일로 분리해 사용하시는 것을 추천드려요!
| const passwordInput = document.getElementById("password"); | ||
| const emailErr = document.getElementById("emailErr"); | ||
| const passwordErr = document.getElementById("passwordErr"); | ||
| const passwordVisibility = document.getElementById("passwordVisibility"); |
There was a problem hiding this comment.
💊 제안
변수명은 해당 변수의 값의 타입을 유추할 수 있는 이름이 좋습니다. isError는 boolean값이라는 것을, emailInput은 input 태그라는 것을 알 수 있습니다. 해당 변수도 emailInput처럼 태그라는 것을 알 수 있게 변경하시는 것을 추천드려요!
| <p id="passwordErr" class="errMsg">비밀번호를 8자 이상 입력해주세요.</p> | ||
| </div> | ||
| </div> | ||
| <button type="submit">로그인</button> |
There was a problem hiding this comment.
❗️ 수정요청
지금은 디자인상 버튼이 회색일 뿐 제출이 되네요.
해당 버튼에 disabled 속성을 추가하셔서 input이 비어있을 때는 버튼이 비활성화되고 클릭되지 않도록 해주세요!
| const signupButton = document.querySelector(".signup"); | ||
| if (isFormValid) { | ||
| signupButton.disabled = false; | ||
| signupButton.classList.add("active"); | ||
| } else { | ||
| signupButton.disabled = true; | ||
| signupButton.classList.remove("active"); | ||
| } |
There was a problem hiding this comment.
💊 제안
위의 조건같은 경우 아래처럼 작성하셔도 됩니다.
const signupButton = document.querySelector(".signup");
signupButton.disabled = !isFormValid;
signupButton.classList.toggle("active", isFormValid);2 participants
요구사항
기본
심화
도록 합니다.
주요 변경사항
스크린샷