[이은서] sprint4#88
Hidden character warning
Conversation
ByungyeonKim
left a comment
ByungyeonKim
left a comment
There was a problem hiding this comment.
고생 많으셨습니다 은서님!
요구사항을 모두 잘 구현해 주셔서, 디테일한 부분들만 피드백 드렸어요.
참고하셔서 리팩토링 해보는 것을 추천드려요!
- 공통 유틸로 분리한 것이 잘 작성된 것인지 궁금합니다.
: 폼 관련 유틸 함수들을 잘 분리해 주셨어요. 자세한 내용은 아래 코드 리뷰에서 확인해 주세요. 😁 - 현재 로그인/회원가입 페이지에서 blur랑 input 이벤트에 유효성 검사를 공통으로 등록하고 있는데, 이벤트 리스너 등록 로직이 페이지마다 거의 반복이라서 이것도 공통으로 분리해볼 수 있을까요?
: 네! 분리해볼 수 있을 것 같네요.setAuthInputListener,setAuthBlurListener로 분리해볼 수 있을 것 같아요.index.js파일에는preventPasswordSpaces(passwordInput);처럼 한 눈에 흐름이 보이도록 만들면 좋을 것 같아요! 불필요한 코드는 줄이고, 관심사 분리를 최대한 해보는거죠. 🤩 고민을 많이 해보시고, 논의가 필요하거나 도움이 필요하면 언제든지 편하게 질문 주세요!
| border-style: none; | ||
| text-decoration: none; |
There was a problem hiding this comment.
불필요한 속성은 최대한 줄여주시는 게 좋아요! 🙂
현재 클래스네임에는 해당 속성이 필요하지 않아요. 사용한 태그에는 보더 스타일이 적용되어 있지 않고, text-decoration: none;은 이미 common.css 파일에 적용되어 있어요.
| min-width: 240px; | ||
| min-height: 48px; |
There was a problem hiding this comment.
이 부분도 이미 뷰포트에 맞게 고정 너비와 높이가 있어서, 제거해도 될 것 같아요. 일반적으로 버튼 UI는 padding 속성으로 크기를 조절하는 것이 좋다고 생각해요.
범용적인 버튼 스타일일 경우에 패딩으로 조절하면 콘텐츠에 맞게 크기가 달라지고, 반응형에 좀 더 유연하다는 장점이 있습니다. 현재는 고정 너비로 잘 처리해 주셨지만, 예기치 못한 상황에서 레이아웃이 깨질 수도 있어요.
| .eye { | ||
| position: absolute; | ||
| width: 24px; | ||
| height: 24px; | ||
| right: 16px; | ||
| top: 50%; | ||
| transform: translateY(-50%); | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
요구사항에 맞게 스타일링을 잘해주셨어요. 여기서 비밀번호가 눈 모양 아이콘과 겹치지 않게 만들어주면 더 좋을 것 같네요! 각자의 영역을 갖도록 하거나, 인풋 태그에 패딩을 줘도 될 것 같아요. 다른 사이트에서 개발자 도구를 켜서, 어떻게 처리를 했는지도 확인해 보세요. 👍 참고하실만한 페이지의 링크를 첨부해 드릴게요.
| .login-btn:hover, | ||
| .header-btn:hover, | ||
| .signup-btn:hover { | ||
| cursor: pointer; | ||
| background-color: #1967d6; | ||
| } | ||
|
|
||
| .login-btn:disabled, .header-btn:disabled { | ||
| background-color: var(--gray400); | ||
| .login-btn:focus, | ||
| .header-btn:focus, | ||
| .signup-btn:focus { | ||
| background-color: #1251aa; | ||
| } | ||
|
|
||
| .login-btn:disabled, | ||
| .header-btn:disabled, | ||
| .signup-btn:disabled { | ||
| background-color: var(--gray400); | ||
| color: var(--white); | ||
| cursor: not-allowed; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
하나의 클래스로 공통 속성을 적용해 보는 것은 어떨까요?
.primary-btn:hover {
cursor: pointer;
background-color: #1967d6;
}이렇게 만들고, 해당 버튼만의 스타일과 공통 버튼 스타일을 적용해 보면 좋을 것 같아요.
<!-- 로그인 버튼 예시 -->
<button class="login-btn primary-btn" type="submit" disabled>
로그인
</button>| <div class="content"> | ||
| <div class="title"> | ||
| <img class="logo" src="/images/pandaicon.png" alt="판다마켓로고" /> | ||
| <a href="/"><h1>판다마켓</h1></a> | ||
| </div> |
There was a problem hiding this comment.
메인 페이지 헤더의 로고처럼, 로그인/회원가입 페이지의 판다마켓 아이콘도 함께 링크 영역으로 만들어서 동작의 일관성을 갖게하는 건 어떨까요?
또한, 로고의 "판다마켓" 텍스트만 글꼴이 달라요. 로고만을 위해 글꼴을 다운받기 보다는, 로고(텍스트 + 아이콘) 자체를 다운받아서 사용하시는 것을 추천드려요! 또한, 이렇게 로고의 크기를 다양하게 사용해야 할 때는 이미지의 크기와 상관없이 이미지가 선명한 svg 포맷을 사용하시면 좋아요. 🙂
| eyeIcon.addEventListener("click", () => { | ||
| eyeIconClickHandler(); | ||
| }); |
There was a problem hiding this comment.
eyeIconClickHandler라는 함수로 한 번 감싸주었기 때문에, 아래처럼 바로 핸들러를 넘겨줘도 됩니다!
| eyeIcon.addEventListener("click", () => { | |
| eyeIconClickHandler(); | |
| }); | |
| eyeIcon.addEventListener('click', eyeIconClickHandler); |
또는, 래핑 함수를 따로 만들지 않고, 리스너의 콜백에서 처리하는 방법도 있겠네요.
| eyeIcon.addEventListener("click", () => { | |
| eyeIconClickHandler(); | |
| }); | |
| eyeIcon.addEventListener('click', () => { | |
| togglePasswordVisiblity(passwordInput, eyeIcon); | |
| }); |
| cursor: not-allowed; | ||
| pointer-events: none; |
There was a problem hiding this comment.
pointer-events: none; 선언은 포인터 이벤트 자체를 없애는 거라서 커서 이벤트가 적용되지 않아요. 둘 중에 하나를 선택해서 적용해 주세요. 🙂
| if (!value) { | ||
| message = "이메일을 입력해주세요."; | ||
| } else if (!emailRegex.test(value)) { | ||
| message = "잘못된 이메일 형식입니다 ."; |
There was a problem hiding this comment.
해당 부분만 마침표 사이에 공백이 있네요!
| }); | ||
|
|
||
| preventPasswordSpaces(passwordInput); | ||
| updateLoginBtnState(); // 초기 상태 업데이트 |
There was a problem hiding this comment.
이미 index.html에서 버튼에 disabled 속성을 줬기 때문에 초기 상태는 비활성화된 상태예요. 해당 코드는 지워도 됩니다!
| passwordInput.addEventListener("keydown", (e) => { | ||
| if (e.key === " " || e.code === "Space") { | ||
| e.preventDefault(); | ||
| } | ||
| }); | ||
|
|
||
| passwordInput.addEventListener("input", () => { | ||
| passwordInput.value = passwordInput.value.replace(/\s/g, ""); | ||
| }); |
There was a problem hiding this comment.
공백 제거 로직을 두 번 작성한 이유가 있을까요? 아래 코드만으로 키 입력과 복사 붙여넣기 둘 다 처리가 가능할 것 같은데 키다운 이벤트도 준 이유가 궁금해요! 🤔
2 participants
요구사항
기본
로그인
회원가입
심화
주요 변경사항
스크린샷
[배포 링크] https://panda-market-silver0.netlify.app/
[로그인]
[회원가입]
멘토에게