-
Notifications
You must be signed in to change notification settings - Fork 21
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 #72
The head ref may contain hidden characters: "Basic-\uC815\uC131\uD604-sprint4"
[정성현] sprint4 #72
Conversation
- min-width CSS 속성으로 화면 최소 너비 적용 - <br> 태그를 통해 줄바꿈 제어 - 주어진 피그마 디자인에 맞도록 일부 CSS 수정
- scripts{ utils } 형태의 JS 파일 구조 생성 - 페이지 이벤트 / 값 검증 용도의 JS 파일 생성 및 연결 - 로그인/회원가입 페이지에 핸들링용 요소 추가
- id는 어떤 요소인지를 나타내는 카멜 케이스로 작명 - name은 담을 내용을 나타내는 카멜 케이스로 작명
- 유효 이메일 여부를 반환하는 함수와, 유효 비밀번호 여부를 반환하는 함수 작성
- 비밀번호 input에 부여된 눈 모양 아이콘 클릭 시 비밀번호 표시 여부와 눈 아이콘 사선 여부가 변경됨
- 입력 요소에서 포커스 아웃 발생 시 유효성 판단 - 판단 결과에 따라 입력 오류 클래스 및 메시지 적용 - 오류를 적용시키는 코드를 errorChange 함수로 분리
- 폼 입력의 유효성을 검증하는 이벤트 작성 - 폼 제출 활성화 판단 함수 작성
- 코드 최적화 진행 - 변수 이동, 변수 삭제, 조건식 bool화
- 폼 제출 시 기본 행동 방지 후 목표 페이지로 이동 - 목표 페이지 HTML 코드 작성
- 에러 <span> 요소 추가를 고려한 CSS 수정 진행
- 대표 이미지 삽입 - 기술 스택 수정 - 네이밍 규칙, 프로젝트 구조 추가
- <label>의 for 값이 <input>의 name을 향하던 것을 <input>의 id를 향하도록 수정
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를 사용하는데 요구사항도 다 구현하시고 좋습니다.
아래는 전반적인 리뷰사항입니다.
- 동일한 사항에 대해서는 한번만 코멘트를 답니다. 비슷한 경우를 찾아서 변경해보세요~
- login_signup.js파일 하나로 login, signup에서의 validation 로직을 같이 처리하셨는데, 공통되는 함수들은 따로 분리하고, login, signUp 로직을 분리하셔도 좋을 것 같습니다.
JS 상에서 폼 내부 요소를 접근하기 위한 방식으로 document.forms[][name]과 탐색 속성(부모, 첫자식, 이전형제 등)을 사용했는데 좀 더 일반화되고 권장된 접근 방식이 있는지 궁금합니다!
: form 요소에 접근해 해당 form의 상호작용가능한 요소들을 가지고 오신것은 좋습니다. 다만 첫자식, 이전형제 등은 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.
P3:
👍
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.
P3:
해당 파일과 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.
스프린트 미션 요구사항에서 폼 제출 시 signin으로 이동시켜달라고 명시해서 임시로 추가했습니다!
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.
아하 그렇군요. 성현님은 signin.html 대신 login.html을 만드셨으니 거기로 이동시키시면 될 것 같습니다~
const email = form["email"]; | ||
const nickname = form["nickname"]; | ||
const password = form["password"]; | ||
const passwordConfirm = form["passwordConfirm"]; | ||
const submit = form.lastElementChild; |
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.
P2:
변수들이 어떤 값을 나타내는지 알 수 있도록 네이밍하시는 것을 추천드립니다.
email보다는 emailInput, submit보다는 submitButton 같은 식이 더 알기 쉽습니다.
// 폼 제출 활성화 판단 | ||
|
||
function updateSubmit() { |
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.
P2:
함수, 변수에 대한 설명 주석은 바로 위에 적어주세요.
// 폼 제출 활성화 판단 | |
function updateSubmit() { | |
// 폼 제출 활성화 판단 | |
function updateSubmit() { |
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.
P2:
button disabled 여부를 js로 컨트롤 하고, html button 요소에 disabled값을 안 주시면 js가 로딩되기 전까지는 버튼이 동작이 가능하니 html button요소에 disbled 속성을 추가해주세요.
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.
P2:
해당 파일을 로그인, 회원가입 페이지에서 공용으로 쓰다보니 함수가 복잡해지는 것 같습니다.
login.js, signup.js를 분리하면 해당 함수도 더 단순해질 것 같습니다.
form.addEventListener("submit", (e) => { | ||
e.preventDefault(); | ||
if (!!nickname) document.location.href = "/signin.html"; | ||
else document.location.href = "/items.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.
P2:
두번 변환하는 것보다 한번만 변환하는 것이 이해하기 쉽습니다. 아래와 같은 방식을 추천드립니다.
form.addEventListener("submit", (e) => { | |
e.preventDefault(); | |
if (!!nickname) document.location.href = "/signin.html"; | |
else document.location.href = "/items.html"; | |
}); | |
form.addEventListener("submit", (e) => { | |
e.preventDefault(); | |
if (!nickname) return document.location.href = "/items.html"; | |
document.location.href = "/signin.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.
조건식에 값이 담길 때, boolean 용도임을 명시하기 위해 !!
를 적용했는데 불필요헀다면 수정하겠습니다!
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.
앗 !
를 한번만 써도 boolean 값으로 변환이 되기 때문에 !nickname => 없을 시 true, 있을 시 false
, !!nickname => 없을 시 false, 있을 시 true
중에 한번만 변환하는게 더 이해하기 쉬울거라고 생각했습니다.
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.
P1:
이벤트를 연결하는 코드들이 함수 선언문 사이사이에 들어가 있어서 코드 파악이 어렵습니다.
가능하면 변수 선언 => 함수 선언 => 이벤트 바인딩문
과 같은 순서처럼 관련있는 코드끼리 기준을 정해서 묶어 작성하시는 것을 추천드립니다.
toggles.forEach((toggle) => { | ||
toggle.addEventListener("click", () => { | ||
const input = toggle.previousElementSibling; | ||
const icon = toggle.firstElementChild; | ||
|
||
if (input.type === "password") { | ||
input.type = "text"; | ||
icon.src = "/images/login_signup/eye_open.svg"; | ||
icon.alt = "비밀번호 가리기"; | ||
} else { | ||
input.type = "password"; | ||
icon.src = "/images/login_signup/eye_close.svg"; | ||
icon.alt = "비밀번호 보기"; | ||
} | ||
}); | ||
}); |
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.
P3:
아래코드도 동일동작합니다.
toggles.forEach((toggle) => { | |
toggle.addEventListener("click", () => { | |
const input = toggle.previousElementSibling; | |
const icon = toggle.firstElementChild; | |
if (input.type === "password") { | |
input.type = "text"; | |
icon.src = "/images/login_signup/eye_open.svg"; | |
icon.alt = "비밀번호 가리기"; | |
} else { | |
input.type = "password"; | |
icon.src = "/images/login_signup/eye_close.svg"; | |
icon.alt = "비밀번호 보기"; | |
} | |
}); | |
}); | |
function togglePasswordButton() { | |
const input = target.previousElementSibling; | |
const icon = target.firstElementChild; | |
const isPasswordInput = input.type === "password" | |
input.type = isPasswordInput ? "text" : "password"; | |
icon.src = isPasswordInput ? "/images/login_signup/eye_open.svg" : "/images/login_signup/eye_close.svg"; | |
icon.alt = isPasswordInput ? "비밀번호 가리기" : "비밀번호 보기"; | |
} | |
toggleButtonList.forEach((target) => { | |
target.addEventListener("click", togglePasswordButton) | |
}); |
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
멘토에게
document.forms[][name]
과 탐색 속성(부모, 첫자식, 이전형제 등)을 사용했는데 좀 더 일반화되고 권장된 접근 방식이 있는지 궁금합니다!