-
Notifications
You must be signed in to change notification settings - Fork 39
[김보경] sprint2 #156
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
[김보경] sprint2 #156
The head ref may contain hidden characters: "Basic-\uAE40\uBCF4\uACBD-sprint2"
Conversation
GANGYIKIM
left a comment
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.
보경님 2번째 미션 제출 고생하셨습니다!
심화 요구사항까지 구현하시다니 멋지네요.
3주차도 화이팅입니다!
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.
❗️ 수정요청
ic_eye_blind와 ic_eye_open 의 이미지 크기가 달라서 비밀번호 토글을 누를때마다 덜컹거리는 느낌이 들어요~
이를 css로 해결하실 수도 있지만 추천드리는 방법은, 두 이미지를 같은 사이즈로 추출하시는 겁니다.
ic_eye_blind가 더 클 수 밖에 없으니 ic_eye_open 이미지를 여백을 주어 동일한 크기로 추출하시는 것을 추천드려요!
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.
💊 제안
css 변수들을 분리하신 점 좋습니다. 다만 figma에 정의된 색상 변수보다 수가 더 적은 것 같아요~
큰 이유가 없다면 figma와 동일하게 색상 변수를 선언하시는 것이 좋습니다.
그래야 figma에서 해당 색상 변수가 사용될때 확인하기 쉽고 색상값은 디자이너와 의사 소통할 일이 많은 값이라 소통 측면에서도 같은 이름을 쓰는 것이 좋기 때문입니다~
또한 클래스로 빼셔도 되지만 css variable로 선언하셔도 됩니다~
:root {
--gray900: #111827;
}
title {
color: var(--gray900);
}| <div class="card-about"> | ||
| <div class="img-about"> | ||
| <img src="./img/Img_home_01.png" /> | ||
| <img src="./img/img_home_01.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.
| <label for="pwcheck">비밀번호 확인</label> | ||
| <input type="text" class="pwcheck" name="pwcheck" id="pwcheck" required /> | ||
| </div> | ||
| <button type="submit" class="signup-button">회원가입</button> |
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.
💬 여담
button 태그의 type 속성들을 잘 명시해주셨어요~
참고로 type이 submit인 button의 경우 form에서 제출 이벤트가 발생하면, 해당 요소의 onclick 이벤트가 실행되게 됩니다.
이를 더 풀어서 말하자면 form 태그안의 input, button 과 같은 상호작용 요소에서 submit 이벤트, enter key 입력이 발생하면 해당 form 에서의 submit 이벤트가 실행된다는 것입니다.
유저 입장에서는 로그인이나 회원가입시 input, button에서 enter를 누르면 제출되는 것에 익숙하기 때문에 이를 신경써주시는 것이 좋습니다~
// case1: form에 onsubmit 이벤트 명시
<form onsubmit="login">
<input /> // input에서 enter 키 입력시 onsubmit 이벤트 실행
<button type="submit">login</button> // 해당 버튼 클릭 시 onsubmit 이벤트 실행
</form>
// case2: submit 버튼에 onclick 이벤트 명시
<form>
<input /> // input에서 enter 키 입력시 button의 onclick 이벤트 실행
<button type="submit" onclick="login">login</button>
</form>| <label for="pwcheck">비밀번호 확인</label> | ||
| <input type="text" class="pwcheck" name="pwcheck" id="pwcheck" required /> |
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.
👍 칭찬
label과 Input을 연결해주신 점 좋습니다~
| #description .wrap-description { | ||
| gap: 69px; | ||
| padding-bottom: 0; | ||
| margin-bottom: 60px; |
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.
❗️ 수정요청
디자인상에서는 해당 요소 하단에 여백이 없으니, 해당 속성은 없는게 좋을 것 같아요.
| margin-bottom: 60px; |
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: center; | ||
| height: 100vh; |
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.
💊 제안
로그인 페이지와 회원 가입 페이지가 디자인이 비슷한만큼 공통적으로 사용하는 css가 많아 보입니다.
이러한 중복을 줄이기 위해 공통 CSS 파일을 작성하시면 유지보수 및 가독성 측면에서 더 좋을 것 같습니다.
auth.css // login, signup 페이지에서 공통으로 쓰는 것들의 모음
login.css
signup.css
만약 login.css, signup.css 파일에서 작성되는 css가 너무 적다면, 즉 분리할 필요를 못 느끼신다면
auth.css에서 전부 작성하셔도 좋을 것 같아요~

요구사항
기본
심화
(추후 클릭으로 비밀번호를 보거나 가릴 수 있도록 기능을 만들어 갈 예정입니다.)
주요 변경사항
스크린샷
멘토에게