-
Notifications
You must be signed in to change notification settings - Fork 39
[유동환] sprint4 #168
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
[유동환] sprint4 #168
The head ref may contain hidden characters: "Basic-\uC720\uB3D9\uD658-sprint4"
Conversation
GANGYIKIM
left a comment
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.
동환님 4번째 미션 제출 수고하셨습니다!
어려워하시더니 잘 작성하셨어요~
배포도 요구사항에 포함된 것으로 압니다. 다음에는 배포 주소도 PR에 같이 올려주시면 더 좋을 것 같아요.
다음 React 미션도 화이팅입니다!
- css 색상변수를 적용하시면서 디자인과 다르게 적용된 곳들이 보이네요! 가능한 디자인과 동일하게 구현해보세요.
- 회원가입 페이지에서 비밀번호 인풋과 비밀번호 확인 인풋의 동작에 대해 더 다양한 케이스를 테스트해보세요. 비밀번호 입력 후 비밀번호 확인 입력하고 다시 비밀번호를 변경하면 비밀번호 확인 인풋은 재확인 로직을 실행하지 않네요.
| <button type="submit" class="login-button buttons"> | ||
| <div class="error-message"></div> | ||
| </div> | ||
| <button type="button" class="login-button buttons" onclick="location.href='./items.html'" disabled> |
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.
❗️ 수정요청
로그인 버튼은 해당 form에서 submit 이벤트에 해당하므로 button type은 submit이 적절합니다.
| <button type="button" class="login-button buttons" onclick="location.href='./items.html'" disabled> | |
| <button type="submit" class="login-button buttons" onclick="location.href='./items.html'" disabled> |
| <a href="./signup.html">회원가입</a> | ||
| </div> | ||
| </container> | ||
| <script src="login.js"></script> |
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.
💊 제안
아마 script 태그를 HTML의 하단에 배치하신 이유가 script가 문서의 렌더링을 막지 않도록 하기 위해서이실 것 같아요.
하지만, script 태그에 defer나 async 속성을 사용하면 이런 문제를 해결할 수 있기 때문에 반드시 하단에 배치할 필요는 없습니다!
또한 script 태그는 상단에 있는게 구조 파악에서도 유리하기 때문에 상단 head 태그에 두시는 것을 추천드려요~
지금과 같은 경우 DOM을 조작하는 JS 이니 defer 속성을 추가하시면 되겠습니다~
| :root { | ||
| --gray50: #F9FAFB; | ||
| --gray100: #F3F4F6; | ||
| --gray200: #E5E7EB; | ||
| --gray400: #9CA3AF; | ||
| --gray500: #6B7280; | ||
| --gray600: #4B5563; | ||
| --gray700: #374151; | ||
| --gray800: #1F2937; | ||
| --gray900: #111827; | ||
| --blue: #3692FF; | ||
| } |
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.
💊 제안
색상변수가 해당 파일과 style.css에 모두 선언되어 있네요.
이런 경우 같은 이름을 가지고 있지만 각각 선언되어 있어서 유지보수 및 작성시 좋지 않습니다!
가능하면 common.css나 variables.css와 같은 파일을 만들어서 모든 페이지에서 공용으로 사용되는 스타일들은 그쪽으로 빼주세요. 참고로 common.css라고 네이밍을 하면 공용코드의 의미이고 variables.css는 색상변수를 모아두는 파일이라는 의미입니다!
| height: 56px; | ||
| border-radius: 40px; | ||
| background-color: #9CA3AF; | ||
| background-color: var(--gray400); |
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.
👍 칭찬
색상 변수 추가하시고 사용하신 것 좋아요!
| const passwordErrorMessage = document.querySelector('.password-field .error-message'); | ||
| const loginButton = document.querySelector(".login-button"); | ||
|
|
||
| const validEmail = /^[0-9a-zA-Z]([-_.]?[0-9a-zA-Z])*@[0-9a-zA-Z]([-_.]?[0-9a-zA-Z])*.(\.[a-zA-Z]{2,3})$/; |
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.
💊 제안
signup.js 파일에서도 동일한 정규식을 사용하고 있는데 그때마다 선언해서 쓰면 문제가 있을 것 같아요.
정규식을 반복해서 선언하면 유지보수가 어려워지고, 코드의 일관성이 떨어질 수 있습니다. 만약 정규식을 수정해야 할 때 여러 곳에서 변경해야 하고 실수로 서로 다른 정규식을 사용할 위험도 커집니다.
이렇게 두개의 파일에서 사용하고 있으니 해당 정규식과 test 함수를 따른 파일로 분리해 사용하시는 것을 추천드려요!
| let isPasswordValid = false; | ||
|
|
||
| function handleFormEmail(event) { | ||
| const input = userEmailField.value; |
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.
💊 제안
input이라는 변수명은 어떤 값인지 알기 어렵고 input 이라는 태그처럼 이해되기 쉬우니 email같이 더 해당값을 잘 설명하는 이름으로 바꿔주시는 것을 추천드립니다.
| function handleLoginButton(event) { | ||
| if ( isEmailValid && isPasswordValid ) { | ||
| loginButton.style.backgroundColor = '#3692FF'; | ||
| loginButton.disabled = false; | ||
| } | ||
| } No newline at end of file |
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.
💊 제안
현재 코드에서는 JavaScript에서 직접 CSS 스타일을 변경하고 있습니다. 하지만 스타일 변경은 가능하면 CSS에서 관리하는 것이 유지보수에 더 용이합니다.
예를 들어, disabled 상태일 때의 스타일을 미리 CSS에서 정의하고, JavaScript에서는 disabled 속성만 제어하는 방식이 더 좋습니다.
.login-button {
background-color: var(--blue);
}
.login-button:disabled {
background-color: #3692FF;
}function handleLoginButton(event) {
loginButton.disabled = !(isEmailValid && isPasswordValid)
}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.
💊 제안
로그인에서 사용하는 js 파일과 에러메시지나 로직 측면에서 동일한 것이 많습니다~
이런 경우 중복을 줄이기 위해 반복 사용되는 함수, 정규식, 에러메시지들은 다른 파일로 분리하시고 각 js 파일에서 가져와서 사용하시는 것을 추천드려요!
| handleSignupButton() | ||
| } | ||
|
|
||
| function handleFormPassword(e) { |
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.
💊 제안
event 인자를 해당 함수내에서 사용하지 않으니 안 받으시는 것이 좋겠습니다~
| function handleFormPassword(e) { | |
| function handleFormPassword() { |
| background-color: #CFE5FF; | ||
| background-color: var(--gray50); |
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.
❗️ 수정요청
gray50 변수의 색상값은 #F9FAFB로 기존값과 다릅니다~ 확인 후 수정해보세요!
요구사항
기본
💡 로그인
💡 회원가입
심화
주요 변경사항
스크린샷
멘토에게