-
Notifications
You must be signed in to change notification settings - Fork 39
[박다인] Sprint1 #5
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
[박다인] Sprint1 #5
The head ref may contain hidden characters: "Basic-\uBC15\uB2E4\uC778-sprint1"
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.
수고하셨습니다! 전체적으로 잘 해주셨는데, 다음에는 반응형을 고려해서 작업 시도해봐요 :)
주요 리뷰 포인트
- 코드 중복 제거
- img alt 태그 사용
- 용도에 따른 css 구분
- 최소너비 화면 잘림 이슈
index.html
Outdated
| </div> | ||
|
|
||
| <section> | ||
| <div class="contents"> |
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.
전체적으로 클래스이름 잘 써주셨는데, 하나만 더 신경써주면 좋을것같아요 :)
contents보다는 feature-contents와 같은 이름으로 지어주면 의미가 명확해지고, 유지보수 관점에서 동일한 클래스이름 중복으로 인한 이슈를 방지해볼수있겠죠?
index.html
Outdated
| </p> | ||
| </div> | ||
| </div> | ||
| <img src="images/home_02.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.
귀찮더라도 img에 alt 사용은 꼭 해주시는게 좋아요!
웹 접근성뿐만 아니라, 이미지 로딩 실패 시 대체 텍스트를 표시하거나 검색엔진최적화에도 도움을 줄 수 있기 때문에 꼭 사용해주시는게 좋습니다.
레퍼런스로 애플 공식 웹사이트에 가서 alt텍스트를 어떻게 사용했는지 참고해보시면:
<img src="/kr/macbook-air/images/overview/design/design_hero_static__e56c1v71mr6u_large.jpg" onload="__lp(event)" alt="열려있는 MacBook Air 13 및 15의 모습. 한 대에는 디자인 작업을 진행 중인 화면이, 다른 한 대에는 이메일과 스프레드시트를 넘나들며 멀티태스킹을 하는 화면이 표시되어 있습니다">이런식으로 alt 속성에 이미지 설명을 위해 구체적이고 명확한 설명을 제공하는 모습을 확인해보실 수 있습니다 :)
| } | ||
|
|
||
| .logo a { | ||
| color: #3692FF; |
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 변수로 관리해볼까요? 코드 중복을 줄이면서 재사용성을 높일수있습니다 :)
:root {
--primary-color: #3692FF;
--background-light: #CFE5FF;
--text-dark: #374151;
--white: #ffffff;
--max-width: 1120px;
}
index.html
Outdated
| </div> | ||
| </header> | ||
| <main> | ||
| <div class="blue"> |
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.
left, blue와 같이 위치나 색상등의 정보를 포함한 css 네이밍은 다소 적절하지않아요.
이유는 디자인이 변경된다면 실제 스타일과 클래스명이 불일치할 수 있고, 이는 곧 재사용성의 저해로 이어지기때문입니다.
따라서, 다음부터는 해당 엘리먼트의 목적과 역할을 보다 직관적으로 이해할 수 있게끔 염두에 두고 작업해보시는걸 추천드립니다 :)
style.css
Outdated
| .blue { | ||
| background-color: #CFE5FF; | ||
| width: 100%; | ||
| min-width: 1920px; |
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.
최소 너비를 1920px로 써주시면 화면 가로 사이즈가 1920px 이하일때 잘려서 보이는 현상이 발생할거예요.
반응형을 고려할때도 좋지않은 선택입니다 :) 해당 클래스가 적용된 클래스에서 어떤 경우에든 부모 컨테이너 너비를 넘어 가로 너비를 꽉 채우는것을 생각하셨다면 이런건 어떨까요?
.full-width {
margin-left: -50vw;
left: 50%;
width: 100vw;
position: relative;
}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.
이렇게 코드를 작성하게 된다면, full-width은 html에서 태그를 한번 더 만들어줘야하는 것일까요?
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.
해당 스타일 규칙을 적용할 엘리먼트에 클래스이름으로 적용해주시면됩니다 :) @canofmato
style.css
Outdated
| width: 100%; | ||
| min-width: 1920px; |
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.
음 min-width: 1920px이 여기저기서 많이 쓰였네요? 이러면 가로 너비가 줄어들때 잘림 현상이 발생될텐데 어떤 의도로 쓰인걸까요? 그리고 동일한 스타일 블락이 꽤 중복되는것 같아요. (header, main section, .blue, footer) 이것도 하나의 클래스로 통합해주면 중복이 좀 사라지겠죠?
예시)
.container {
width: 100%;
min-width: 1920px;
display: flex;
justify-content: center;
}
.center-content {
width: 100%;
max-width: var(--max-width);
display: flex;
justify-content: space-between;
align-items: center;
}
style.css
Outdated
| .panda.description { | ||
| padding-bottom: 60px; | ||
| gap: 32px; | ||
| } |
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.
NIT: 지금도 깔끔하게 구조화가 되어있어 보기 좋지만, 좀더 의미가 명확하고 직관적인 클래스이름을 부여해주는건 어떨까요?
.description {
color: #374151;
gap: 24px;
}
.description--panda { /* BEM 적용 */
padding-bottom: 60px;
gap: 32px;
}이런식으로 BEM을 적용하다보면
.description h1 {
font-size: 40px;
font-weight: 700;
line-height: 140%;
}이런 코드도 아래처럼 좀더 의미있고 명확한 클래스명을 가지게끔 보여줄수있어요.
.description__title {
font-size: 40px;
font-weight: 700;
line-height: 140%;
}| * { | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| html { | ||
| font-family: Pretendard, sans-serif; | ||
| } | ||
|
|
||
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
|
|
||
| } |
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 reset/ normalize 용도의 파일로 따로 분리해서 import해서 사용하는건 어떨까요?
- reset.css: 스타일 초기화
- common.css: 전역 스타일 처리
이렇게 용도에 따라 css 파일을 나눠 사용해보는 경험을 늘려봅시다! :)
질문에 대한 답변브라우저가 커짐에는 문제가 없으나 작아지는데에 있어서 페이지가 같이 작아지는것이 아닌 잘리는 것처럼 구현이 되는 것 같습니다.네, min-width: 1920px 을 적용했기에 1920px보다 가로 너비가 작아지면 화면 잘림이 발생하고있습니다 :) 거듭해서 코드를 수정하다보니 크기에 대한 부분이 많이 섞인 것 같습니다. 처음에는 브라우저가 크고 작아져도 페이지가 같이 크고 작아졌던 것 같은데 어느순간부턴 섞여서 고정이 되어버린 것 같습니다.다음부터는 반응형 대응을 위해 브레이크포인트별로 스타일 규칙 명명해주는걸 목표로 작업해봅시다! |
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게