-
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 #70
The head ref may contain hidden characters: "part1-\uAE40\uC9C0\uC724-sprint4"
[김지윤]sprint4 #70
Conversation
각 요소에 padding으로 배치 -> 최대 너비 1200px로 할당 후 중앙 정렬 -> 두 번째 요소만 기본축 end 정렬
미디어 쿼리 값과 기본 값 바뀌어서 변경
전체 페이지 중앙 정렬 삭제 -> 로그인-회원가입 페이지 로고 위 공통 margin: 60px 할당
input의 font-family: Pretendard 추가 -> 브라우저 기본 값 때문에 적용이 안 되어서 직접 할당 placeholder의 line-height 삭제 -> chrome의 기본 값 !important 때문에 line-height 적용 안 됨 아이콘 CDN 링크 삭제
로고 클래스명 겹침으로 인한 크기 오류 -> 랜딩 페이지 / 로그인-회원가입 페이지를 기준으로 클래스명을 분리 --> 랜딩 페이지의 로고는 재사용 가능성이 있으므로, 로그인-회원가입 페이지만 변경 로고 이미지 깨짐 현상으로 lg 사이즈 이미지로 전부 변경 -> 기존 사이즈별 이미지 전부 삭제 -> lg 이미지 적용 후 각 조건에 맞게 너비와 높이 지정 랜딩 페이지 break-point: 375px 기준 로고 변경을 위한 이미지 파일 추가
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 조건문에서 조건을 변수로 빼서 변수명을 보고 어떤 경우 로직이 실행되는지 알 수 있게 작업해주신것이 좋았습니다.
- 네이밍이 아쉬운 부분들이 있습니다. 이름을 보고 어떤 동작을 하는 함수인지, 어떤 변수인지, 요소인지, 불린값인지, text인지 등 명확하게 작성해주시면 가독성에 많은 도움이 될 것 같습니다.
<img | ||
class="login-form__icon js-pw-icon" | ||
src="/images/icon/pw-hidden.png" | ||
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.
P2:
상호작용 요소이므로 button 태그를 추천드립니다.
<img | |
class="login-form__icon js-pw-icon" | |
src="/images/icon/pw-hidden.png" | |
alt="비밀번호 숨기기 버튼" | |
/> | |
<button type='button' class='js-pw-icon'> | |
<img | |
class="login-form__icon" | |
src="/images/icon/pw-hidden.png" | |
alt="비밀번호 숨기기 버튼" | |
/> | |
</button> |
<script type="module" src="auth-validation.js"></script> | ||
<script type="module" src="toggle-pw.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.
P2:
HTML 문서 다운로드 후 JS가 다운로드 되게 하단에 배치하신 것 같은데
하단에 배치된 스크립트는 구문분석 순서상 추후에 다운로드 되게 만들어주지만 일반적으로 스크립트 태그가 위치되는 head 요소가 아니므로 가독성 측면에서 단점이 있고 이러한 동작은 스크립트 자체 속성을 통해 이끌어 낼 수 있으므로 script async, defer 속성 사용을 추천드립니다.
https://developer.mozilla.org/ko/docs/Web/HTML/Element/script#defer
https://ko.javascript.info/script-async-defer
const hidden = "/images/icon/pw-hidden.png"; | ||
const view = "/images/icon/pw-view.png"; | ||
|
||
function toggleIcon(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.
P2:
안쓰는 인자의 경우 지워주시는 것이 가독성에 더 좋을 것 같습니다.
function toggleIcon(e) { | |
function toggleIcon() { |
export const pw = document.querySelector(".js-pw"); | ||
const pwIcon = document.querySelector(".js-pw-icon"); | ||
|
||
const hidden = "/images/icon/pw-hidden.png"; | ||
const view = "/images/icon/pw-view.png"; |
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:
해당 변수들은 하단의 toggleIcon함수에서만 사용되므로 내부에서 선언해주시면 더 좋을 것 같습니다.
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:
아래처럼 작성하셔도 동일동작합니다~
const pwIcon = document.querySelector(".js-pw-icon");
function toggleIcon() {
const pwInput = document.querySelector(".js-pw");
const hiddenIcon = "/images/icon/pw-hidden.png";
const viewIcon = "/images/icon/pw-view.png";
const isPasswordType = pwInput.type === "password";
pwInput.type = isPasswordType ? "text" : "password";
pwIcon.src = isPasswordType ? viewIcon : hiddenIcon;
pwIcon.alt = isPasswordType ? "비밀번호 보기 버튼" : "비밀번호 숨기기 버튼";
pwInput.focus();
}
pwIcon.addEventListener("click", toggleIcon);
const pwIcon = document.querySelector(".js-pw-icon");
function toggleIcon() {
const pwInput = document.querySelector(".js-pw");
if(pwInput.type === "password") {
pw.type = "text";
pwIcon.src = "/images/icon/pw-view.png";
pwIcon.alt = "비밀번호 보기 버튼";
} else {
pw.type = "password";
pwIcon.src = "/images/icon/pw-hidden.png";
pwIcon.alt = "비밀번호 숨기기 버튼";
}
pwInput.focus();
}
pwIcon.addEventListener("click", toggleIcon);
const button = document.querySelector(".js-button"); | ||
const emailError = document.querySelector(".js-email-error"); | ||
const pwError = document.querySelector(".js-pw-error"); | ||
const nicknameError = document.querySelector(".js-nickname-error"); |
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:
해당 변수도 errorMessage가 들어갈 요소이니 아래와 같은 이름이 더 유의미할 것 같습니다.
const nicknameError = document.querySelector(".js-nickname-error"); | |
const nicknameErrorContainer = document.querySelector(".js-nickname-error"); |
const pwError = document.querySelector(".js-pw-error"); | ||
const nicknameError = document.querySelector(".js-nickname-error"); | ||
const pwCheckError = document.querySelector(".js-pw-check-error"); | ||
const pwCheck = document.querySelector(".js-pw-check"); |
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:
const pwCheck = document.querySelector(".js-pw-check"); | |
const pwCheckInput = document.querySelector(".js-pw-check"); |
// 유효성 검사 결과 | ||
const PASSED = 0; | ||
const FAILED = 1; | ||
let valueStatus = []; |
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:
valueStatus
를 배열로 관리해야하는 이유를 잘 모르겠습니다.
해당 변수 사용처를 보면 에러가 났을 때, 0 혹은 1을 추가해 buttonActivation
함수에서 해당 변수의 1, FAILED가 있는지 확인하는 로직인데, error가 얼마나 많이 났는지 여부가 중요하지 않기 때문에 boolean 값으로 관리하셔도 될 것 같습니다.
function valueCheck(e) { | ||
!e.target.value ? setInvalid(e.target) : setValid(e.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.
P2:
함수의 경우 동사가 먼저 오는 것이 일반적이라 아래와 같은 이름을 추천드립니다.
function valueCheck(e) { | |
!e.target.value ? setInvalid(e.target) : setValid(e.target); | |
} | |
function checkValue(e) { | |
!e.target.value ? setInvalid(e.target) : setValid(e.target); | |
} |
function pwValidation(el) { | ||
if (!el.value) { | ||
if (el.name === "password") { | ||
setInvalid(el, pwError, "비밀번호를 입력해주세요"); | ||
} else { | ||
setInvalid(el, pwCheckError, "비밀번호를 입력해주세요"); | ||
} | ||
valueStatus.push(FAILED); | ||
} else if (el.value.length < 8) { | ||
if (el.name === "password") { | ||
setInvalid(el, pwError, "비밀번호를 8자 이상 입력해주세요"); | ||
} else { | ||
setInvalid(el, pwCheckError, "비밀번호를 8자 이상 입력해주세요"); | ||
} | ||
valueStatus.push(FAILED); | ||
} else { | ||
setValid(el, el.name === "password" ? pwError : pwCheckError); | ||
valueStatus.push(PASSED); | ||
} | ||
} |
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:
if문 중첩은 가독성에 좋지 않습니다.
early return 문을 이용하거나, 조건문을 변수에 할당해 코드의 가독성을 높힐 수 있습니다.
function pwValidation(el) { | |
if (!el.value) { | |
if (el.name === "password") { | |
setInvalid(el, pwError, "비밀번호를 입력해주세요"); | |
} else { | |
setInvalid(el, pwCheckError, "비밀번호를 입력해주세요"); | |
} | |
valueStatus.push(FAILED); | |
} else if (el.value.length < 8) { | |
if (el.name === "password") { | |
setInvalid(el, pwError, "비밀번호를 8자 이상 입력해주세요"); | |
} else { | |
setInvalid(el, pwCheckError, "비밀번호를 8자 이상 입력해주세요"); | |
} | |
valueStatus.push(FAILED); | |
} else { | |
setValid(el, el.name === "password" ? pwError : pwCheckError); | |
valueStatus.push(PASSED); | |
} | |
} | |
function validatePassword(el) { | |
const isValid = el.value && el.value.length >= 8; | |
const errorTarget = !el.value ? pwError : pwCheckError; | |
const errorMessage = !el.value | |
? "비밀번호를 입력해주세요" | |
: "비밀번호를 8자 이상 입력해주세요"; | |
if (isValid) { | |
setValid(el, el.name === "password" ? pwError : pwCheckError); | |
valueStatus.push(PASSED); | |
return; | |
} | |
setInvalid(el, errorTarget, errorMessage); | |
valueStatus.push(FAILED); | |
} | |
7b63826
into
codeit-bootcamp-frontend:Basic-김지윤
요구사항
Github에 PR(Pull Request)을 만들어서 미션을 제출합니다.
피그마 디자인에 맞게 페이지를 만들어 주세요.
React와 같은 UI 라이브러리를 사용하지 않고 진행합니다.
기본
로그인
회원가입
심화
멘토에게
배포
스프린트 미션 4