-
Notifications
You must be signed in to change notification settings - Fork 39
[이찬호] Sprint4 #159
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 #159
The head ref may contain hidden characters: "Basic-\uC774\uCC2C\uD638-sprint4"
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.
찬호님 4번째 미션 제출 고생하셨습니다~
잘 정리해주셔서 의도가 잘 보이는 코드였어요.
저도 제가 이해하는 한도에서는 찬호님의 의도를 존중하며 피드백드렸습니다~
다음 React 미션도 화이팅이에요!
-
기존 유틸리티 기반 CSS에 익숙하여 라이브러리없이 유사한 접근으로 유틸리티 클래스를 별도 파일로 분리하여 사용하였는데 해당 구조가 적절한지, 아니면 다른 구조(각 파일에 유틸리티를 포함)가 더 나을지 조언 부탁드립니다.:
우선 고려하고 계신 다른구조인 각 파일에 유틸리티 클래스를 포함하는 것보다는 지금의 방식이 더 유틸리티 클래스라는 접근법에 적절하다고 생각합니다.
저도 현업에서 tailwind를 사용했고 그 방식을 좋아합니다만 반응형 작업을 위해서는 처음에 많은 유틸 클래스들이 필요합니다. 지금 찬호님이 작성하신 것만으로는 부족할 것 같아요. 만약 이러한 방식을 쓰실거라면 tailwind를 참고해보시거나 미션5부터는 tailwind를 설치하셔서 적용하시는 것이 좋지 않을까합니다. -
처음부터 html 에서 button에 disabled 속성을 true로 주고 시작하는게 더 좋을 것 같아요. 지금처럼 JS에서 컨트롤해야할 이유를 잘 모르겠고 이렇게 되면 HTML 에서 button이 활성화된채로 렌더링됩니다.
| .button:disabled { | ||
| background-color: var(--gray-400); | ||
| cursor: default; | ||
| pointer-events: none; | ||
| } |
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.
💊 제안
하단에 btn-disabled라는 클래스를 통해 구현하신 것을 봤는데, 유틸 클래스로 나누실 거라도 기본적인 것들은 공통으로 작성하시는 것이 더 좋아보여요. 그래서 cursor같은 값들은 그냥 button:disabled 에 기본적으로 추가해주시는 것을 추천드려요.
| margin-left: 16px; | ||
| } | ||
|
|
||
| h1 { |
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.
💊 제안
찬호님께서 잘 작성하셨겠지만 염려가 되어 코멘트 남깁니다~
태그 선택자는 해당 태그가 모든 페이지에 걸쳐 적용되어야 하는 스타일링 외에는 사용을 추천드리지 않습니다~
위의 경우도 프로젝트가 확장되다보면 변경될 수 있으니 가능한 class를 사용하시는 것을 추천드릴께요~
| <script src="scripts/validation.js"></script> | ||
| <script src="scripts/signup.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 속성을 추가하시면 되겠습니다~
| function isValidPasswordLength(password, minLength = 8) { | ||
| return password.length >= minLength; | ||
| } |
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.
💊 제안
추후 확작성을 고려하셔서 작성해주신 것은 좋으나 이렇게 되면 요구사항상 비밀번호 min값인 8에 대한 명확성이 코드상 떨어집니다.
저라면 minLength는 필요할때 추가하시거나 8이라는 수를 상수로 가지고 계셔서 조금 더 명확하게 작성하시는 것을 추천드려요.
| function isValidPasswordLength(password, minLength = 8) { | |
| return password.length >= minLength; | |
| } | |
| export const MIN_PASSWORD_LENGTH = 8; | |
| function isValidPasswordLength(password, minLength) { // default 값으로는 사용처에서 알 수 없음. | |
| return password.length >= minLength; | |
| } | |
| const isPasswordValid = isValidPasswordLength(input, MIN_PASSWORD_LENGTH); |
| function showError(inputElement, message) { | ||
| const container = inputElement.closest(".input-item"); | ||
|
|
||
| const existingError = container.querySelector('[class*="text-error"]'); | ||
| if (existingError) { | ||
| existingError.remove(); | ||
| } | ||
|
|
||
| inputElement.classList.add("input-invalid"); | ||
|
|
||
| const errorSpan = document.createElement("span"); | ||
| errorSpan.className = "text-error text-sm block mt-2 font-semibold"; | ||
| errorSpan.textContent = message; | ||
|
|
||
| if (inputElement.parentElement.classList.contains("input-wrapper")) { | ||
| inputElement.parentElement.parentElement.appendChild(errorSpan); | ||
| } else { | ||
| container.appendChild(errorSpan); | ||
| } | ||
| } |
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.
💊 제안
이미 에러 메시지 태그가 존재할때 지우고 다시 생성하시는 것은 DOM 조작을 여러번하게됩니다.
에러가 존재한다면 내부 문자열만 바꿔주는 방식이 성능 및 가독성에도 더 좋을 것 같습니다.
| function showError(inputElement, message) { | |
| const container = inputElement.closest(".input-item"); | |
| const existingError = container.querySelector('[class*="text-error"]'); | |
| if (existingError) { | |
| existingError.remove(); | |
| } | |
| inputElement.classList.add("input-invalid"); | |
| const errorSpan = document.createElement("span"); | |
| errorSpan.className = "text-error text-sm block mt-2 font-semibold"; | |
| errorSpan.textContent = message; | |
| if (inputElement.parentElement.classList.contains("input-wrapper")) { | |
| inputElement.parentElement.parentElement.appendChild(errorSpan); | |
| } else { | |
| container.appendChild(errorSpan); | |
| } | |
| } | |
| function showError(inputElement, message) { | |
| const container = inputElement.closest(".input-item"); | |
| let errorSpan = container.querySelector(".text-error"); | |
| // error 태그 생성하는 부분은 함수로 분리해도 좋을듯 합니다. | |
| if (!errorSpan) { | |
| errorSpan = document.createElement("span"); | |
| errorSpan.className = "text-error text-sm block mt-2 font-semibold"; | |
| container.appendChild(errorSpan); | |
| } | |
| inputElement.classList.add("input-invalid"); | |
| errorSpan.textContent = message; | |
| } |
| function setButtonState(buttonElement, isEnabled) { | ||
| if (isEnabled) { | ||
| buttonElement.disabled = false; | ||
| buttonElement.classList.remove("btn-disabled"); | ||
| } else { | ||
| buttonElement.disabled = true; | ||
| buttonElement.classList.add("btn-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.
💊 제안
if 문보다 아래처럼 작성하시는 것이 더 가독성 측면에서 좋을 것 같아요!
| function setButtonState(buttonElement, isEnabled) { | |
| if (isEnabled) { | |
| buttonElement.disabled = false; | |
| buttonElement.classList.remove("btn-disabled"); | |
| } else { | |
| buttonElement.disabled = true; | |
| buttonElement.classList.add("btn-disabled"); | |
| } | |
| } | |
| function setButtonState(buttonElement, isEnabled) { | |
| buttonElement.disabled = !isEnabled; | |
| buttonElement.classList.toggle("btn-disabled", !isEnabled); | |
| } |
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.
👍 칭찬
중복을 줄이시고자 로그인과 회원가입 페이지에서 중복 로직을 분리하신점 좋아요!
| if (!signupButton.disabled) { | ||
| window.location.href = "login.html"; | ||
| } |
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이 disabled 상태라 제출이 되지 않으므로 관련 코드는 없어도 될 것 같아요~
| if (!signupButton.disabled) { | |
| window.location.href = "login.html"; | |
| } | |
| window.location.href = "login.html"; |
| function validatePasswordMatch() { | ||
| if (isNotEmpty(passwordConfirmInput.value)) { | ||
| if (!isMatching(passwordInput.value, passwordConfirmInput.value)) { | ||
| showError(passwordConfirmInput, "비밀번호가 일치하지 않습니다."); | ||
| } else { | ||
| removeError(passwordConfirmInput); | ||
| } | ||
| } | ||
| } |
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.
💊 제안
if문이 중첩되면 가독성이 떨어지므로 아래처럼 작성하시는 것을 추천드립니다.
| function validatePasswordMatch() { | |
| if (isNotEmpty(passwordConfirmInput.value)) { | |
| if (!isMatching(passwordInput.value, passwordConfirmInput.value)) { | |
| showError(passwordConfirmInput, "비밀번호가 일치하지 않습니다."); | |
| } else { | |
| removeError(passwordConfirmInput); | |
| } | |
| } | |
| } | |
| function validatePasswordMatch() { | |
| if (!isNotEmpty(passwordConfirmInput.value)) return; | |
| if (!isMatching(passwordInput.value, passwordConfirmInput.value)) { | |
| showError(passwordConfirmInput, "비밀번호가 일치하지 않습니다."); | |
| } else { | |
| removeError(passwordConfirmInput); | |
| } | |
| } |
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
배포주소: https://leech-spfe15-panda-market.netlify.app/
기본 요구사항
체크리스트 [기본]
로그인
회원가입
체크리스트 [심화]
주요 변경사항
스크린샷
멘토에게