-
Notifications
You must be signed in to change notification settings - Fork 26
[노준서] sprint3 #77
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
[노준서] sprint3 #77
The head ref may contain hidden characters: "Basic-\uB178\uC900\uC11C-sprint3"
Conversation
baeggmin
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.
지난 리뷰가 꼼꼼히 반영되었네요! 덕분에 코드가 많이 정리된 것 같습니다. 👍
다만 아직 폴더 구조가 정리되지 않아서 이 부분은 꼭 수정해주시면 좋을 것 같네요. 자세한 리뷰는 인라인 코멘트 참고해주세요.
| width: 1120px; | ||
| margin: 0 auto; | ||
| width: 100%; | ||
| /* margin: 0 auto; */ |
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.
사용하지 않는 코드는 제거해주세요!
지금은 학습단계이기때문에 크게 문제되지 않지만, 추후 팀 프로젝트에서는 다른 팀원들에게 혼란을 줄 수 있습니다.
| .panda_bottom_left p { | ||
| padding-bottom: 60px; | ||
| gap: 10px; | ||
| width: auto; |
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.
height: auto, width: auto는 기본값이기 때문에 불필요한 경우가 많습니다. 한 번 auto 속성 없이도 디자인에 문제 없는지 확인해주시고, 가능하면 제거해주세요!
| .login-container { | ||
| width: 100%; | ||
| max-width: 400px; | ||
| padding: auto 16px; |
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.
padding 속성에 auto는 유효하지 않은 값입니다.
브라우저가 무시하는 값이니 padding: 0 16px; 로 수정 부탁드립니다.
| flex-direction: column; | ||
| width: auto; | ||
| } | ||
| .panda_bottom p { |
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.
width, height, text-align을 동시에 지정하는 방식은 비효율적인거같아요.
텍스트 정렬이 목적이라면 text-align만으로 충분합니다!
| } | ||
|
|
||
| /* Mobile (375~767px) */ | ||
| @media (max-width: 767px) and (min-width: 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.
브레이크 포인트 범위를 명확히 지정해서 디바이스 타입에 따라 레이아웃을 구분해주신 점 훌륭합니다. 👍
다만, Tablet과 Mobile 미디어 쿼리 내 코드가 거의 동일해서 중복 코드가 많이 생긴 것 같아요.
중복되는 부분은 공통 클래스로 추출한 뒤, 차이점만 미디어 쿼리에서 오버라이드하시면 코드가 훨씬 깔끔해질 것 같습니다.
(예시)
.section_card {
width: 100%;
display: flex;
flex-direction: column;
align-items: center;
padding: 20px;
}
@media (max-width: 1199px) {
.section_card.middle {
flex-direction: column-reverse;
text-align: right;
}
}
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.
제가 지난 PR 에서 놓친 것 같은데요, 현재 프로젝트 구조가 다소 부적절한 것 같습니다.
.github 디렉토리는 GitHub 전용 설정용 폴더입니다 (e.g., workflows/, ISSUE_TEMPLATE/ 등).
html, css 같은 소스 파일은 일반 웹 프로젝트 폴더로 변경해주셔야 합니다. 아래 구조를 참고해주세요!
├── index.html
├── login.html
├── signup.html
├── css/
│ ├── reset.css
│ ├── base.css
│ ├── home.css
│ ├── form.css
├── images/
│ ├── logo.png
│ ├── icon_visibility_on.svg
│ ├── icon_visibility_off.svg
├── js/
│ └── (login.js 등 추가되면 여기에)
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 파일이 많지만 이름이 구체적이지 않고 역할이 겹쳐 보입니다.
basictag.css 는 어떤 역할인지 불명확하므로, reset.css, base.css 등 명확한 이름으로 수정해주세요!
style.css 도 메인인지 전체인지 애매하므로 home.css 또는 main.css로 바뀌면 좋겠습니다.
login_signup.css와 같이 파일에 여러 페이지명을 나열하는 방식은 보편적인 방식은 아닌 것 같습니다. form.css 로 수정되면 좋을 것 같습니다.
| } | ||
| .section_card { | ||
| width: 100%; | ||
| flex-direction: column; |
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.
각 섹션을 flex-direction: column, flex-direction: row로 디바이스별로 다르게 지정해주신 점 좋습니다! 👏
|
👉 rem 단위: 글자 크기, 버튼 패딩, 여백 등 "정해진 비례가 필요한 요소"에 주로 사용 저의 경우에는 보통 아래와 같이 반응형 레이아웃을 구현하는 것 같아요. 참고해보시고 작은 부분부터 적용해보시면 좋을 것 같습니다.
|
요구사항
기본
랜딩 페이지
로그인, 회원가입 페이지 공
심화
주요 변경사항
스크린샷
멘토에게