-
Notifications
You must be signed in to change notification settings - Fork 39
[홍성현] sprint3 #131
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 #131
The head ref may contain hidden characters: "Basic-\uD64D\uC131\uD604-sprint3"
Conversation
addiescode-sj
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.
수고하셨습니다!
주요 리뷰 포인트
- 미디어쿼리 사용 관련 피드백
- 이미지 리소스 최적화 관련 피드백
| /* login signup mobile */ | ||
| @media (min-width: 375px) and (max-width: 767px) { | ||
| .login-container, | ||
| .signup-container { | ||
| margin: 80px 16px 231px; | ||
| max-width: 400px; | ||
| } | ||
| } | ||
|
|
||
| /* login signup Tablet */ | ||
| @media (min-width: 768px) and (max-width: 1199px) { | ||
| .login-container, | ||
| .signup-container { | ||
| margin: 0 52px; | ||
| } | ||
| } |
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.
기본 스타일을 제일 작은 화면 (모바일)에 맞춰 작성하면 @media (min-width: 375px) and (max-width: 767px) 해당 미디어쿼리 평가문에서 스타일을 재정의할 필요가 없어질것같아요!
index.html
Outdated
| <div class="hero__content"> | ||
| <h2 class="hero__title">일상의 모든 물건을<br />거래해 보세요</h2> | ||
| <h2 class="hero__title"> | ||
| 일상의 모든 물건을<br class="onlyPC" />거래해 보세요 |
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.
미디어 쿼리로 br 태그의 표시 여부를 제어하는 방식을 사용하고있는데, 단순 줄바꿈을 위해서 이런식으로 처리하기보다는
CSS 속성을 사용해 제어해주는게 더 깔끔한 방식일것같아요.
우선 줄바꿈이 필요한 위치에 Enter를 넣어주시고
CSS에서는 이런식으로 whitespace 속성을 조절하는건 어떨까요?
@media (max-width: 767px) {
.hero__title {
/* 모바일에서는 텍스트가 음절 단위로 자연스럽게 줄바꿈 */
word-break: keep-all;
width: 100%;
}
}
@media (min-width: 768px) {
.hero__title {
/* PC에서는 원하는 지점(Enter가 삽입된 지점)에서 줄바꿈 */
white-space: pre-line;
}
}
login.html
Outdated
| <picture class="header__login-image"> | ||
| <source | ||
| srcset="/assets/images/logos/panda-logo.png" | ||
| media="(max-width: 767px)" | ||
| /> | ||
| <img | ||
| src="/assets/images/logos/panda-logo-lg.png" | ||
| alt="판다 마켓의 상징적인 판다 얼굴 로고" | ||
| /> | ||
| </picture> |
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.
picture태그를 사용하셔도 괜찮지만 뷰포트에 따라 같은 이미지소스를 해상도만 다르게 최적화해서 보여주는 방식은 srcset, sizes 만으로도 충분합니다 :)
덧붙여, 첫 화면에 보이는 이미지가 아닌 스크롤을 쭉 내려야 보이는 이미지라면 레이지로딩을 적용할수도있고요 :)
아래 아티클 참고해보시고 리팩토링해볼까요?
signup.html
Outdated
| <link rel="stylesheet" href="./assets/css/variables.css" /> | ||
| <link rel="stylesheet" href="./assets/css/global.css" /> | ||
| <link rel="stylesheet" href="./assets/css/style.css" /> | ||
| <link rel="stylesheet" href="./assets/css/pages/login-signup.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.
login-signup.css 보다는 auth.css 정도가 좀 더 확장성있는 네이밍을 고려했을때 나을것같네요 :)
질문에 대한 답변
font-size를 rem으로 쓸것인지 px로 쓸것인지 먼저 정하시고 해당 값이 여러 페이지에서 자주 쓰이는 값이라면 변수화하시면됩니다. PR 본문 내에서 미디어쿼리 사용 관련해서 자세히 피드백 드렸습니다 :) 불필요한 스타일 재정의를 최소화하기위해서는 보통은 모바일 퍼스트로 접근하는게 좋다고 피드백드린거라 참고해보시면 좋을것같네요~ |
공통
랜딩 페이지
로그인,회원가입 페이지 공통
심화
스크린샷
멘토에게
*** login.css와 signup.css를 분리하려고 했으나 아직까지는 필요성을 느끼지 못하여 같이 쓰고 있습니다