[이태경] Sprint1 #2
Conversation
addiescode-sj
requested review from
addiescode-sj
and removed request for
bootcampCodeit
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
전체적으로 작업도 꼼꼼하게 하셨고, 시맨틱 태그 & 구조화에 대해 신경써서 처리하신게 보이네요.
잘하셨습니다! 👍 NIT: 가 달려있는 코멘트는 학습 목적으로만 참고해보셔도 되고, 나머지 피드백도 보시면서 좀 더 개선해봐요!
주요 리뷰 포인트
- 접근성 향상
- id와 class 사용처
- 모호한 클래스명 수정
- css 변수 사용
- 용도에 따른 파일 구분
| <img | ||
| src="assets/images/main/main_service_search.png" | ||
| alt="" | ||
| /> |
There was a problem hiding this comment.
NIT: 귀찮더라도 이미지에 alt 속성을 부여해주세요!
추가적으로, 이미지 최적화를 위해서는 width와 height를 속성으로 부여해주시는게 좀 더 좋습니다.
성능 최적화 정도를 나타내는 중요한 지표중에 CLS(Cumulative Layout Shift)가 있는데요,
만약 width와 height가 HTML 속성으로 부여되어있지않다면 CSS 파일이 로드되기전에는 브라우저가 이미지의 크기를 알 수 없고, CSS 파일은 나중에 로드되거나 변경될 수 있기때문에 선택자 매칭과 계산 과정이 필요하게됩니다.
따라서 레이아웃이 갑자기 변경될 수 있는 가능성을 방지(CLS 방지)하면서 CSS가 로드되지않더라도 항상 안정적으로 레이아웃을 유지시키고싶다면, HTML 속성으로 width와 height를 미리 지정해 브라우저에게 이미지 로드전에 필요한 정확한 크기 계산(레이아웃 계산)을 수행하게 만들어줄수있습니다. 물론 이정도로 가시적이고 유의미한 차이는 없겠지만, 이 기회에 브라우저에서 html, css, js파일이 어떻게 로드되는지 동작 원리나 배경을 학습해보시면 좋을것같아 알려드립니다! :)
index.html
Outdated
| <a href="/" | ||
| ><img src="assets/images/common/logo.svg" alt="판다마켓 로고" | ||
| /></a> |
There was a problem hiding this comment.
접근성 향상을 위해 aria-label을 붙이는 습관을 들여보면 어떨까요?
시각 장애인, 시력 저하 사용자들은 스크린 리더를 통해 웹페이지를 사용하게되는데, 이때 aria-label이 제공하는 텍스트를 읽어주게됩니다. 따라서 링크 이동 시 "판다마켓 홈으로 이동" 이라는 텍스트를 듣게된다면 해당 링크가 사용된 목적을 좀 더 명확히 전달할수있겠죠?
There was a problem hiding this comment.
예시: <a href="/" aria-label="판다마켓 홈으로 이동">
| ><img src="assets/images/common/logo.svg" alt="판다마켓 로고" | ||
| /></a> | ||
| </h1> | ||
| <div class="link_area"> |
There was a problem hiding this comment.
link_area의 경우 해당 파일에서 한번 사용되었는데, 16번째 라인처럼 id를 사용하지않은 이유가 있을까요? 이번 기회로 id와 class 사용처에 대한 기준을 명확히 가져가보는게 어떨까요?
There was a problem hiding this comment.
강사님 안녕하세요:) 우선, 코드리뷰 감사합니다!
이건 제가 제 생각을 남겨야 될거 같아서, 코멘트를 달아봅니다 ㅎㅎ
확장성이나 유지보수면을 생각했을 땐, 알려주신 BEM 방법론과 함께 쓴다면, class가 조금 더 스타일을 입히기 위해서는 좋다고 생각을 해서 사용하게 되었습니다! (여기선 BEM 방법론을 쓰지 않았지만요..!)
사실 id는 getElementById로 요소를 찾아 조작해야할 때 써야 한다고 생각해서 이렇게 작성을 했습니다!
그래도 코드리뷰 보고 큰 영역에 대해서는 id를 쓰는게 맞는거 같다는 생각이 들어서 개선해서 반영했습니다.
혹시 제 생각이 틀린게 있다면 가감없이 알려주세요! 감사합니다:)
There was a problem hiding this comment.
큰 영역이라기보단, 고유한 영역 (예를 들자면, 여러 페이지에 걸쳐 스타일링을 한다고 가정할때 딱 하나밖에 없는 요소)를 식별하기위해 사용하시는게 좋아요. 이해하신것처럼 요소간 계층에 대한 위계가 잘 드러나기만한다면 사실상 id 선택자를 사용할 필요가 없어집니다.
참고해보세요! ;) @LeeTaegyung
index.html
Outdated
| <!-- header :: E --> | ||
| <main class="main"> | ||
| <!-- visual_sec :: S --> | ||
| <section class="visual_sec"> |
There was a problem hiding this comment.
모호한 클래스명으로 느껴집니다. 일단 visual이라는 의미 자체가 일반적인 느낌이 드네요 :)
모호한 클래스명은 가독성, 유지보수성, 확장성이 모두 떨어질 수 있습니다. 유지&보수하는 입장에서 클래스명을 보고 어떤 역할을 하는 기능/컴포넌트인지 충분히 이해하기 어려워 관리에 어려움이 생길 수 있고, 확장하기도 어려워지고, 지금 같은 경우에는 나중에 다른 개발자가 동일한 클래스명으로 스타일을 또 만들면 스타일이 충돌되거나 꼬이는 문제가 생길 수도 있기 때문입니다.
이런 문제를 최대한 방지하기위해서는 클래스명을 잘 지어주시는게 좋습니다.
네이밍 컨벤션을 검색해보시고, 가장 일반적으로 BEM(Block Element Modifier)을 많이 사용하는편이라 학습 후 적용해보시는걸 추천드릴게요! :)
assets/css/style.css
Outdated
| @@ -0,0 +1,382 @@ | |||
| /* ====== reset ====== */ | |||
There was a problem hiding this comment.
용도를 구분해서 주석을 잘 넣어주셨네요! 잘하셨습니다 :)
다만 이런 초기화 코드는 나중에 다른 html이나 css파일을 만들때 복사 붙여넣기하듯이 그대로 가져다쓰는게아니라, reset.css와 같이 따로 파일을 만들어서 필요한 파일에서 import해서 사용하는 방식이 좋지 않을까요 ? :) 초기화 목적뿐만이 아니라, 전역적인 스타일 처리를 해주는 파일도 따로 만들면 훨씬 관리가 수월해질거예요!
assets/css/style.css
Outdated
| padding: 0 5px; | ||
| color: #fff; | ||
| font-weight: 600; | ||
| background: #3692ff; |
There was a problem hiding this comment.
이렇게 반복적으로 사용되는 값들은 변수로 처리해서 재사용해주는게 코드 중복도 줄일 수 있고 관리도 수월할거예요. 예시를 드려보겠습니다 :)
:root {
/* Colors */
--primary-color: #3692ff;
--primary-bg: #cfe5ff;
--white: #fff;
--dark-bg: #111827;
--text-light: #9ca3af;
--text-dark: #000;
/* Layout */
--container-width: 1120px;
--container-margin: 200px;
--header-height: 70px;
--hero-height: 540px;
--card-height: 444px;
--footer-height: 160px;
/* Spacing */
--spacing-xs: 8px;
--spacing-sm: 12px;
--spacing-md: 24px;
--spacing-lg: 32px;
--spacing-xl: 64px;
--spacing-xxl: 124px;
/* Border Radius */
--radius-sm: 8px;
--radius-md: 12px;
--radius-lg: 40px;
/* Font Sizes */
--text-sm: 18px;
--text-base: 24px;
--text-lg: 40px;
}
assets/css/style.css
Outdated
| background: #fcfcfc; | ||
| } | ||
|
|
||
| .btn { |
There was a problem hiding this comment.
버튼도 상태/조건에 따라 클래스이름과 스타일 규칙을 잘 구분해주셨네요! 잘하셨습니다 👍
질문에 대한 답변기본 요청 사항중 "화면의 너비가 1920px 보다 작아질 때, "판다마켓" 로고의 왼쪽 여백 200px"로그인" 버튼의 오른쪽 여백 200px이 유지되고, 화면의 너비가 작아질수록 두 요소간 거리가 가까워지도록 설정합니다." 는 의도한 바가 이게 맞는지 모르겠습니다.우선 작업된걸 보니 calc로 양쪽 여백을 제외하고 고정 width값을 준거라서 요구사항을 충족시키진않네요! 로컬 레포지토리에 스프린트 미션별로 폴더를 만들어서 관리를 해야 할까요?코드잇 미션 제출 매뉴얼 그대로 따라가주시면됩니다~ 스프린트 미션 가이드에 PR시 commit 메세지는 refactor(mentor): var를 let으로 수정 으로 통일해달라고 하셨는데, 모든 커밋메세지를 그렇게 하면 되는건가요? 아님, PR을 위해 push전에 올리는 커밋 메세지만 refactor(mentor): var를 let으로 수정 로 하면 되는건가요?PR 받고나서 수정 작업하실때 refactor(mentor): 라벨을 붙인 형태의 커밋 메시지 형식으로 작성해달라는게 의도였을것같네요! :) |
2 participants
요구사항
기본
심화
스크린샷
멘토에게
refactor(mentor): var를 let으로 수정으로 통일해달라고 하셨는데, 모든 커밋메세지를 그렇게 하면 되는건가요? 아님, PR을 위해 push전에 올리는 커밋 메세지만refactor(mentor): var를 let으로 수정로 하면 되는건가요?