-
Notifications
You must be signed in to change notification settings - Fork 39
[배민지] sprint1 #40
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 #40
The head ref may contain hidden characters: "Basic-\uBC30\uBBFC\uC9C0-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.
수고하셨습니다!
전체적으로 포맷팅과 클래스이름 작명하는것 조금 더 신경써서 작업해보면 좋을것같아요.
이번에 드린 피드백 잘 공부해보시고 개선해보세요~!
주요 리뷰 포인트
- 포맷팅
- 클래스이름 수정
- 유지보수/재사용성 고려한 대응
- 코드 중복 제거
index.html
Outdated
| <a href="/"> | ||
| <img class="title" src="Group 19.png"> | ||
| </a> |
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.
접근성을 고려해 <a href="/" aria-label="홈으로 이동"> 과 같이 aria-label 혹은 role을 추가해볼까요?
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.
모든 a태그에 적절히 적용하였습니다
index.html
Outdated
|
|
||
|
|
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.
불필요한 공백은 지웠습니다
index.html
Outdated
| </a> | ||
|
|
||
| </div> | ||
| <img style="width: 746px;" src="Img_home_top.png" alt="판다가 반기는 이미지"> |
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.
오호, width 값을 style 속성을 통해 넣어주셨군요.
style 속성으로 width만 지정하면, 이미지 로딩 전까지 높이를 알 수 없어 레이아웃이 변할 수 있어요. 대신 width와 height 속성을 직접 지정하면 브라우저가 이미지의 종횡비를 즉시 계산할 수 있어 최적화에 도움이 됩니다.
앞으로 이미지 최적화도 조금 더 신경써볼까요?
웹페이지 성능 측정 지표중 CLS(Cumulative Layout Shift) 라는 지표가 있는데요,
현재 보고 있는 페이지에 갑자기 발생하는 레이아웃의 변경이 발생하면 사용자 입장에서 불편함을 겪을 수 있기때문에 레이아웃 이동(layout shift) 빈도를 측정해 성능을 평가한다고 보시면 됩니다. 보통 이미지 및 비디오 요소를 삽입할때 이런 현상이 자주 발생될수있겠죠?
따라서 레이아웃 시프트를 방지해주려면 이미지 및 비디오 요소에 width 와 height 속성을 항상 포함하거나 또는 CSS를 사용하여 필요한 공간(aspect-ratio-box)을 잡는 방법이 있습니다.
해당 이미지의 사이즈가 어느정도는 고정적이라면 width와 height를 직접적으로 넣는 방법이 있고, 반응형을 고려해 비율을 지키며 변해야한다면 aspect-ratio를 사용하는 방법도 있습니다.
이미지 최적화는 제가 제안드린 방법말고도 webp, avif 같은 차세대 확장자를 사용한다던지
srcSet을 사용해 각 스크린사이즈에 맞는 이미지파일을 로드한다던지
우선순위를 낮춰 레이지 로딩하는 등 여러가지 기법들이 있습니다.
차차 학습해보시면 좋을것같네요 👍
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.
따로 학습해보겠습니다!
index.html
Outdated
| </header> | ||
|
|
||
|
|
||
| <div class="skyBlue"> |
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.
시맨틱 HTML 태그 사용을 늘려볼까요? (<main>, <section>, <nav> 등)
아래 아티클을 읽어보시면 시맨틱 태그를 왜 사용해야하는지 잘 참고하실수있을거예요!
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.
위 아래 배경이 하늘색인부분을 시맨틱태그 < section>로 수정했습니다
index.html
Outdated
|
|
||
|
|
||
| <div class="skyBlue"> | ||
| <div class="upAndDown"> |
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.
클래스이름을 지을때는 UI의 색상, 위치, 순서등이 포함되어있으면 디자인 변경 시 대응하기도 어려워지고 유지보수에 좋지 않아요. 이런 정보들을 포함하기보다는 항상 엘리먼트의 역할을 명확하게 나타낼수있는 네이밍을 써볼까요? :)
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.
skyBlue를 용도에 맞게 highlightBox로 수정하였습니다.
index.html
Outdated
| </div> | ||
|
|
||
| <div class="icons"> | ||
| <a href="https://www.facebook.com/?locale=ko_KR"> |
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.
외부 링크의 경우, 보안을 위해 rel="noopener noreferrer"를 추가해주세요! :)
noopener: 새 탭에서 열린 페이지가 원본 페이지에 접근하는 것을 방지
noreferrer: 리퍼러 정보가 새 페이지로 전달되는 것을 방지
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.
외부 링크로 연결되는 4가지 링크에 모두 적용하였습니다
style.css
Outdated
| @@ -0,0 +1,189 @@ | |||
| *{ | |||
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.css: CSS 초기화
- common.css: 전역 스타일
이렇게 용도에 따라 파일을 나눠 관리하는건 어떤 이점을 가져다줄수있는지 한번 더 생각해볼수있어요.
- 역할 분리가 되어 관리가 쉬움: 각 파일의 목적이 명확해서 수정이나 유지보수가 쉬워질거예요.
- 사용성과 확장성 증가: 여러 페이지 혹은 프로젝트에서 거의 동일하게 재사용이 가능해요.
- 스타일 우선순위가 더 예측 가능해짐: reset.css → 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.
common.css: 전역 스타일 파일을 추가하였고
html에도 < link rel="stylesheet" href="common.css">를 추가해 주었습니다
| height: auto; | ||
| } | ||
|
|
||
| .top{ |
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.
여기서도 .top 뒤에 공백 한칸 띄워주세요! 다음부터는 포맷팅 꼭 신경써봐요 :)
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에서 클래스/태그와 대괄호 사이에 띄어쓰기를 추가했습니다
|
|
||
| body{ | ||
| font-family: 'Pretendard', sans-serif; | ||
| color: #374151; |
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;
--text-color: #374151;
--background-color: #ffffff;
--secondary-background: #FCFCFC;
--footer-background: #111827;
--footer-text: #9CA3AF;
--hero-background: #CFE5FF;
--container-width: 1920px;
--section-padding: 100px 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.
미션 2부터 적용해보겠습니다 ㅎㅎ
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
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.
요렇게 자주 반복되는 스타일 블락들도 유틸리티 클래스로 만들어서 재사용해줄 수 있어요!
예시)
.flex-center {
display: flex;
justify-content: center;
align-items: center;
}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.
이것도 미션 2부터 고려해서 적용해보겠습니다
요구사항
기본
지도록 해주세요.
심화
주요 변경사항
배포 링크
https://pandamarket9029.netlify.app/
멘토에게