-
Notifications
You must be signed in to change notification settings - Fork 39
[송시은] Sprint4 #101
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 #101
The head ref may contain hidden characters: "Basic-\uC1A1\uC2DC\uC740-sprint4"
Conversation
91144a3 to
3181f21
Compare
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번째 미션 제출 고생하셨습니다!
-
비밀번호 노출 버튼을 토글시 이미지가 덜컹거리는 느낌이드네요~ 두 이미지의 높이가 달라서 그런것으로 보이니 높이를 같게해서 토글시 이미지가 안 움직이게 추출하시는 것을 추천드려요.
-
회원가입 페이지에서 비밀번호 입력 후 비밀번호 확인 입력하고 다시 비밀번호를 변경하면 비밀번호 확인 인풋은 재확인 로직을 실행하지 않네요~ 한번 다양한 케이스를 테스트해보세요.
-
텍스트의 경우, div안에 그냥 태그 없이 넣은 것도 있는데 span태그로라도 감싸는게 좋을까요?: 어디를 말씀하시는지 모르겠습니다만 의미없이 태그를 추가할 필요는 없어보입니다. -
로고 텍스트를 이미지로 변경 후 반응형에서 크기 변경을 width로 했는데 맞게 한건가요?: 넵 -
이벤트 핸들러 작성 시, 함수 정의까지 addEventListener안에서 해야하는지 아니면 정의는 밖에서 해야하는지 정하는 기준이 궁금합니다. 유지보수 측면에서 모든 함수를 다 분리하고 이벤트핸들러에는 이름으로 등록 하려다가 그게 더 복잡한거 같아서 바꿨는데 어떤 기준에서 해야할까요? 예를 들어서 아래의 경우 이벤트 핸들러를 밖에서 선언하고 가져오는게 나을까요 아니면 그냥 아래처럼 안에서 선언하는게 나을까요?:
이는 취향에 따라 갈릴 것 같습니다만 저는 재사용되지 않는 로직이라면 이벤트 핸들러 함수 내부에서, 재사용되는 로직이거나 외부로 빼는것이 가독성측면에서 더 유리하다면 분리할 것 같습니다. -
DOM객체를 ID로 생성할 경우 querySelector보다는 getElementById를 사용하는 것이 미미하지만 성능에 더 좋다고 해서 querySelector와 getElementById를 함께 사용했는데 성능차이가 미미하니까 일관성을 위해 querySelector로 바꾸는게 좋을까요?:
바꾸시나 안 바꾸시나 일관성 측면이나 성능 측면에서 큰 차이가 없을 것 같아서 제 기준에서는 크게 상관 없을 것 같습니다. -
리드미를 만들고 나름대로 컨벤션을 정해봤는데 함수규칙을 아래와 같이 해도 괜찮을까요? 함수규칙은 안하는게 나을까요?:
규칙이 있는 것이 더 좋다고 생각합니다. 지금 작성해주신 규칙은 일반적으로 사용되는 규칙이라고 생각합니다~ -
유효성 검사 부분을 좀 복잡하게 구현한 것 같기도 하고 중복 되는 부분이 있는 것 같기도 하고 뭔가 아쉽습니다. if를 많이 사용했는데 괜찮은건지도 궁금합니다. 코드도 나누는게 더 좋은 부분이 있는지 궁금합니다.: 코멘트 참고해주세요~ -
DOMContentLoaded 이벤트핸들러 안에 코드를 작성했는데 이렇게 하는게 나을까요 아니면 DOMContentLoaded를 사용하지 않는게 좋을까요?:
스크립트를 모듈로 불러오셔서 자동으로 지연 실행되므로 DOMContentLoaded 이벤트 핸들러에 등록하시나 하지 않으시나 동일동작합니다. 따라서 크게 상관 없을 것 같습니다.
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.
👍 칭찬
자세히 작성하신 것 좋습니다~
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.
👍 칭찬
클래스명이 더 깔끔해졌네요~
| <div class="login-signup-container"> | ||
| <header class="login-signup-header"> | ||
| <body class="auth-body"> | ||
| <script type="module" src="../index.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 태그는 상단에 있는게 구조 파악측면에서 유리하다고 생각해서 큰 이유가 없다면 상단 head 태그에 두시는 것을 추천드려요~
|
|
||
| /* | ||
| responsive CSS | ||
| desktop-first(1920이상) 큰 모니터 |
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.
👍 칭찬
사용하시는 반응형 작성 방법론도 아시고 좋네요~ mobile-first가 대세라고 하지만 저는 desktop-first가 편하더라구요~
| const validateEmail = (email) => { | ||
| const regex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| return regex.test(String(email)); | ||
| }; |
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만 검증하고 에러 메시지 표시 | ||
| const validateInput = (target) => { |
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들을 나눠서 작성해주시면 더 깔끔할 것 같아요!
| const inputContainer = targetInput.closest('.input-container'); | ||
| if (!inputContainer) return; | ||
| const errorContainer = inputContainer.querySelector( | ||
| '.validation-error-message', | ||
| ); |
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.
💊 제안
inputContainer 변수가 한번만 사용되고 있어 변수로 저장할 필요성이 낮아보여요~
아래처럼 작성하시면 더 가독성측면에서 좋을 것 같아요~
변수는 최소 2번 이상 사용하거나 변수로 선언했을 때 가독성에서 유리할 것 같은 경우만 선언하시는 것을 추천드려요!
| const inputContainer = targetInput.closest('.input-container'); | |
| if (!inputContainer) return; | |
| const errorContainer = inputContainer.querySelector( | |
| '.validation-error-message', | |
| ); | |
| const errorContainer = targetInput.closest('.input-container')?.querySelector('.validation-error-message'); | |
| if (!errorContainer) return; |
| const updateSubmitButtonState = () => { | ||
| authSubmitButton.disabled = !validateForm(); | ||
| }; | ||
|
|
||
| // 포커스아웃 시 인풋 유효성 검사 | ||
| form.addEventListener('focusout', function (event) { | ||
| if (event.target.matches('input')) { | ||
| validateInput(event.target); | ||
| } | ||
| }); |
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 (validateForm()) { | ||
| if (authType === 'signup') { |
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 문의 중첩을 피하실 수 있어요~
| if (validateForm()) { | |
| if (authType === 'signup') { | |
| if (!validateForm()) return; | |
| if (authType === 'signup') { |
…c-송시은-sprint4 [송시은] Sprint4
…c-송시은-sprint4 [송시은] Sprint4

요구사항
기본
배포 링크
https://codeit-sprint15-mission.netlify.app/
로그인
회원가입
심화
주요 변경사항 (이전 미션 부분)
(회원가입, 로그인 페이지 내용물 크기가 작은 모니터와 태블릿 화면 사이즈에서 동일하게 하기 위해)
스크린샷
멘토에게
예를 들어서 아래의 경우 이벤트 핸들러를 밖에서 선언하고 가져오는게 나을까요 아니면 그냥 아래처럼 안에서 선언하는게 나을까요?
DOM객체를 ID로 생성할 경우 querySelector보다는 getElementById를 사용하는 것이 미미하지만 성능에 더 좋다고 해서 querySelector와 getElementById를 함께 사용했는데 성능차이가 미미하니까 일관성을 위해 querySelector로 바꾸는게 좋을까요?
리드미를 만들고 나름대로 컨벤션을 정해봤는데 함수규칙을 아래와 같이 해도 괜찮을까요? 함수규칙은 안하는게 나을까요?
개선할 수 있는 부분이 있다면 알려주시면 감사하겠습니다. 🙂