-
Notifications
You must be signed in to change notification settings - Fork 44
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
[홍진호] week3 #103
The head ref may contain hidden characters: "part1-\uD64D\uC9C4\uD638-week3"
[홍진호] week3 #103
Conversation
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.
@ugaugaugaugaugauga
과제 작업하시느라 고생하셨습니다!
확실히 다양한 하이엔드 라이브러리 활용 경험이 있으셔서 그런지 화려하게 잘 되어있네요 ㅎㅎ
조금 더 욕심 내어보자면, 진호님께서는 활용법을 잘 이해하시는 만큼, 어떻게 이런 구조가 되었는지에 대해 조금 더 파보면 훨씬 좋은 프로젝트가 될 것 같아요.
그런 관점에서, CSS구조와 프로젝트 파일을 다음과 같이 분리해 관리해보면 어떨까요?
index.html - 메인 랜딩페이지
index.css - 메인 랜딩페이지에서 사용될 스타일
styles/
- common.css - 공통 활용될 스타일 (css reset / variable 등)
- ui.css - 공통 활용될 UI 스타일 (container, text-primary, btn 등)
signup/ - 회원가입 페이지
- index.html
- style.css
login/ - 로그인 페이지
- index.html
- style.css
assets/
images/
- 이미지들 ...
특히, common.css들과 ui용 css, 그리고 각 파일마다 활용될 css를 분리해 작업하는게 조금 더 필요해보입니다. 이 부분 한번 참고해서 작업해보시구,
bem 네이밍 컨벤션도 한번 참고해 조금 더 빡세게 적용해 활용해보시면 어떨까요? 구조 및 구현의 화려함도 좋지만, 결국 컨벤션을 잘 지켜 읽기 쉽고 관리하기 좋은 코드를 작성하는게 더 중요하니깐요.
프로젝트 진행하시면서 아리송하거나 막히는 부분 있으시면 언제든 물어봐주셔요.
고생 많으셨습니다!
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.
@ugaugaugaugaugauga
컨테이너만 이렇게 모아두는건 조금 이상해보입니다.
특히, 컨테이너의 목적은 단순히 최대 넓이값을 설정하고 좌우 패딩을 설정하며 해당 콘텐츠가 페이지의 중앙에 위치하도록 위치시키는 작업만 하기 때문에 지금 컨테이너들은 너무 많은 일들을 하고 너무 다양해보입니다.
따라서 좀 더 재활용 가능한 요소들만 공통 class로 만들어 common.css
등에 모아두는식으로 두는게 좋겠어요!
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 파일 구조를 용도에 맞게 분리시키고 (예를들어 기존의 container, global, style -> common.css(공용) page의 세부적인 디자인은 각 page의 style.css에 적용)
ui.css는 공용으로 사용하는 btn, container, input 스타일 같은 용도로 사용하는군요!
추가로 bem 에 맞게 네이밍도 다시 조정해보겠습니다! ㅎㅎ
다음 커밋에는 피드벡을 수용하여 최대한 노력해서 작성해보겠습니다!
추후에 구조 변경을 하는과정에서 모르는 부분이 있거나 햇갈리는 부분이 있으면discord DM으로 정리하여서 질문을 하겠습니다.
다시한번 좋은 피드벡과 칭찬에 감사드립니다.
font-family: 'Pretendard-Regular'; | ||
src: url('https://cdn.jsdelivr.net/gh/Project-Noonnu/noonfonts_2107@1.1/Pretendard-Regular.woff') | ||
format('woff'); | ||
font-weight: 400; | ||
font-style: normal; |
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.
@ugaugaugaugaugauga
font famliy설정은 body에서 설정하고, 나머지 요소들은 Inherit을 받아와 활용하도록 하는게 좋아보입니다.
또한 font의 fallback설정도 필요하구요.
그리고 웹폰트 활용 시 font-face를 활용하는식으로 구현되는게 좋겠습니다!
https://css-tricks.com/snippets/css/using-font-face-in-css/#aa-practical-level-of-browser-support
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.
@ugaugaugaugaugauga
이 요소들은 style.css
라기보단 여기저기서 공통으로 활용될 수 있는 요소들이죠?
그래서 common.css
정도 파일에 관리해주는건 어떨까요?
.hero-header { | ||
margin-top: 40px; | ||
text-align: center; | ||
font-size: 32px; | ||
font-weight: 700; | ||
} |
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.
@ugaugaugaugaugauga
이 요소는 공통으로 활용될 여지가 없어보이는데요
랜딩페이지 외에 사용되지 않는다면 ui
요소로 두기보단, 해당 페이지에서만 적용할 스타일시트에 작성하는게 좋을 것 같아요
.auth-input { | ||
margin-top: 12px; | ||
padding: 18px 15px; | ||
border: 1px var(--gray-400) solid; | ||
border-radius: 8px; | ||
} | ||
|
||
.auth-input:focus { | ||
outline: none; | ||
border-color: var(--primary); | ||
} |
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.
@ugaugaugaugaugauga
auth input의 경우, 로그인 / 회원가입창의 인풋 요소로 보이는데요
이 녀석들도 해당 페이지에서만 임포트 될 수 있도록 스타일시트 분리하는건 어떨까요?
그리구 auth-input이라는 이름보단,
전략을 세워본다면
모든 input을 동일한 스타일로 적용시키고
bem을 활용해 해당 인풋들만 타겟해 스타일을 추가시키는게 좋아보입니다.
요구사항
기본
[site link] (https://bright-snickerdoodle-1ec20a.netlify.app/)
심화
주요 변경사항
스크린샷
멘토에게