Conversation
CJewon
requested review from
bootcampCodeit
and removed request for
bootcampCodeit
CJewon
added
help wanted
가로길이가 1920px이 넘어가는게 잘못된건 아닌걸로 보여요. 무엇이 문제인지 조금 더 맥락을 포함해서 설명 부탁드립니다 |
There was a problem hiding this comment.
제원님! 고생 많으셨습니다.
전반적으로 마크업에 대한 고민이 많이 보이는 PR이였어요.
아무래도 쉽지 않았을텐데 고민한 영역들이 많이 보여서 - 특히 nth child나 section태그를 활용하는 방법 등 - 보기 좋은 프로젝트였습니다.
다만 이번 프로젝트에서 몇가지 개선할 부분이 있어요.
전반적으로 스타일 시스템을 구조화 하는 방법과 적합한 마크업 사용 방법만 개선된다면 훨씬 좋은 프로젝트가 되리라 생각합니다.
이에 대해서는 코드단 리뷰에 남겨두었으니 한번 참고 부탁드려요.
고생 많으셨습니다
There was a problem hiding this comment.
css reset을 적용하셨군요! 잘 하셨습니다
img/판다 얼굴.png
Outdated
There was a problem hiding this comment.
파일 이름이는 되도록 영어 소문자와 _ 언더스코어만 활용해주세요.
프로젝트를 호스팅하는 서버 운영체제에 따라 영문자 외의 낱말을 처리하는 방식이 조금씩 다른 경우가 있습니다. 따라서 영문자가 아니거나 띄어쓰기를 포함하는 경우 의도치 않은 오류를 발생시킬 여지가 있어요
| <header> | ||
| <div class="header-container"> | ||
| <div class="logo-container"> | ||
| <img src="/img/판다 얼굴.png" alt="판다얼굴" /> | ||
| <a href="/">판다마켓</a> | ||
| </div> | ||
| <a href="/login" class="login-button">로그인</a> | ||
| </div> | ||
| </header> |
There was a problem hiding this comment.
구조와 시멘틱성은 기본적으로 잘 잡혀 있습니다.
그러나 클래스 명명 규칙과 BEM에 일관성을 부여하고, HTML 요소의 사용을 조금 개선할 여지가 있습니다.
1. BEM 네이밍
header-container, logo-container와 같은 클래스명은 명확성을 위해 header__container 및 header__logo-container처럼 BEM 규칙을 따르는 것이 좋습니다. 또한 login-button 클래스는 header의 하위 요소임을 명확히 하려면 header__login-button과 같은 BEM 방식으로 수정할 수 있습니다.
조금 더 나아가 말씀드리자면 header 영역 안에 로고가 존재하고 로그인 버튼이 존재하죠? 따라서 header header__logo header__login 등으로 분리를 해보면 어떨까 생각합니다.
2. 시멘틱 구조 개선
img와 a 요소가 logo-container에 함께 있는데, 이 경우 img 요소에 alt 텍스트를 통해 logo의 의미를 더 명확히 표현하는 것도 좋습니다.
또한, 이 둘을 태그로 묶어 로고 클릭 시 홈으로 이동하도록 하는 방식이 더 직관적입니다.
There was a problem hiding this comment.
위 설명을 토대로 다음을 참고해 코드를 개선해보면 어떨까요?
<header class="header">
<div class="header__container">
<a href="/" class="header__logo">
<img class="header__logo-img" src="/img/판다 얼굴.png" alt="판다마켓 로고" />
<span class="header__logo-text">판다마켓</span>
</a>
<a href="/login" class="header__login-button">로그인</a>
</div>
</header>| <a href="/login" class="login-button">로그인</a> | ||
| </div> | ||
| </header> | ||
| <section class="main-section"> |
There was a problem hiding this comment.
섹션태그를 적용하셨군요! 잘 하셨습니다.
섹션은 특별하게 이 영역부터 끝나는 부분까지가 하나의 묶음이라는 설명을 하기위해 활용되는 태그에요.
| <div class="margin-container"> | ||
| <div class="main-banner-container"> | ||
| <div class="click-container"> | ||
| <h2>일상의 모든 물건을<br />거래해 보세요</h2> | ||
| <a href="/items">구경하러 가기</a> | ||
| </div> | ||
| <img src="/img/Img_home_top.png" alt="Img-home " /> | ||
| </div> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
다만 BEM 관점에서 다음 내용들을 참고해보면 더 좋은 코드로 될 수 있지 않을까 생각해요
일단 배너 영역을 작업한 내용인데 배너임을 나타내는 클래스명이 존재하지 않아 구조와 명확성이 조금 부족합니다.
1. BEM 네이밍 및 배너 역할 명확화
section 태그를 사용했고 main-section이라는 클래스명을 통해 페이지의 주요 섹션임을 최대한 표현해주셨어요. 그럼에도 불구하고 배너에 대한 구체성이 부족합니다. main-banner와 같이 배너를 명확히 가리키는 클래스명을 사용하면 배너 역할이 더 분명해집니다.
또한 margin-container와 click-container도 역할이 명확히 잘 보이지 않습니다.
2. 시멘틱 구조 개선
h2 요소를 통해 우리 제품이 전달하고자 하는 핵심 메시지를 작성해주셨어요.
다만 우려되는 사항으로 홈페이지가 전달하고자 하는 핵심 이야기는 h1태그를 사용해 강력하게 어필하는걸 추천합니다.
보통 h1태그는 브랜드 네임이나 브랜드가 전달하고자 하는 핵심 메시지를 표현할때 사용하는데요.
아무래도 우리 홈페이지를 한 단어로 설명하기 위한 요소에 활용하다보니 제 생각에는 지금 h2로 담긴 콘텐츠가 h1으로 제공되는게 더 좋지 않을까 싶습니다.
또한 헤더 영역을 지나 우리 홈페이지의 메인 콘텐츠가 시작되는데요. main태그로 감싸주어 헤더 이후에 들어오는 콘텐츠들이 메인 콘텐츠임을 보여주면 어떨까요?
There was a problem hiding this comment.
다음 코드를 한번 참고해서 개선시켜보면 어떨까 생각합니다
<section class="main-banner">
<div class="main-banner__container"> /** 여기서 최대넓이, 포지셔닝 등을 설정합니다 */
<div class="main-banner__text-container">
<h1 class="main-banner__title">일상의 모든 물건을<br />거래해 보세요</h1>
<a href="/items" class="main-banner__cta">구경하러 가기</a>
</div>
<img src="/img/Img_home_top.png" alt="홈 페이지 상단 배너 이미지" class="main-banner__img" />
</div>
</section>| header, | ||
| section { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
기본적으로 모든 block 요소는 width: 100%설정을 지니고 있습니다. 굳이 명세할 필요는 없어보여요
|
|
||
| a { | ||
| text-decoration: none; | ||
| text-align: center; |
There was a problem hiding this comment.
링크 요소가 상당히 다양한 상황에서 사용될 수 있을텐데요.
모든 링크에게 중앙정렬을 하기보단, 중앙 정렬이 되어야만 하는 링크에게 text-align을 적용해주세요
| .logo-container a { | ||
| font-size: 26px; | ||
| font-weight: 700; | ||
| color: #3692ff; | ||
| } |
| h2 { | ||
| font-size: 40px; | ||
| font-weight: 700; | ||
| color: #374151; | ||
| line-height: 56px; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
html 태그중에는 특별한 목표를 지닌 태그들이 존재합니다.
특히 heading 태그들은 스타일적인 목적보단 정보 위계를 보여주기 위해 사용하곤 합니다.
따라서 h태그에게 직접 스타일을 적용하기보단, h태그가 사용할 클래스를 할당해 스타일을 지정하는게 좋습니다.
이 관점에서 BEM 형식을 활용하는게 아무래도 도움이 많이 되는데, 한번 구조를 고민해볼까요?
| .footer-ul > li:first-child { | ||
| color: #9ca3af; | ||
| } | ||
|
|
||
| .footer-ul > li:nth-child(2) { | ||
| display: flex; | ||
| gap: 30px; | ||
| } | ||
|
|
||
| .footer-ul > li:nth-child(2) > a { | ||
| color: #e5e7eb; | ||
| } |
* reset * fix: 머지 후 브랜치 삭제 github action 수정 * env: workflows 폴더로 이동 * codeit sprint mission part1 update (#13) * BEM 네이밍 및 시멘틱 구조 개선 * BEM에 맞게 HTML의 class 명칭과 CSS 명칭 수정 * codeit-sprint2 mission 로그인 홈페이지와 회원가입 홈페이지 제작 (#68) * [최제원] Sprint2 (#102) * div태그 대신 header 태그로 변경 및 logo-img에 대한 a태그 추가 * 이미지 파일구조 수정 및 파일구조 수정에 따른 이미지들의 src 수정 * login.html form태그와 label태그로 변경 및 비밀번호 가림 이미지 추가와 그에 맞는 css 적용 * signup.html form태그와 label태그로 변경 및 비밀번호 가림 이미지 추가와 그에 맞는 css 적용 * [최제원] Sprint3 (#114) * 메인 홈페이지 반응형 웹으로 변경함으로써 CSS 수정 및 변경 * login 홈페이지와 signup 홈페이지를 반응형 웹으로 변경함으로써 CSS 수정 및 변경 * 불필요한 CSS 삭제 및 BEM에 맞게 클래스 이름 수정 * 로그인 홈페이지와 회원가입 홈페이지의 에러메세지 관련 로직 구현 * input에 aria-label 속성 추가 --------- Co-authored-by: hanseulhee <3021062@gmail.com> Co-authored-by: withyj-codeit <48437814+withyj-codeit@users.noreply.github.com>
요구사항
기본
지도록 해주세요.
심화
주요 변경사항
스크린샷
멘토에게