-
Notifications
You must be signed in to change notification settings - Fork 39
[박다인] Sprint2 #29
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
[박다인] Sprint2 #29
The head ref may contain hidden characters: "Basic-\uBC15\uB2E4\uC778-sprint2"
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.
수고하셨습니다!
저번 리뷰때 드린 코멘트들 잘 반영해서 훨씬 클래스이름 사용하시는게 능숙해지셨네요 :)
이번 리뷰에는 접근성, 성능, 유지보수등 여러가지 다른 개선점들을 알려드려봤어요.
다듬어서 멋지게 프로젝트 마무리해봅시다!
주요 리뷰 포인트
- 이미지 최적화
- 접근성 향상
- 네이밍케이스, 네이밍컨벤션
| @@ -1,216 +1,142 @@ | |||
| :root { | |||
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.
컬러 코드뿐만 아니라, 레이아웃이나 여백 관련해 자주쓰는 속성들도 최대한 추가해볼까요?
이렇게 추가한 변수들을 반응형을 고려할때도 미디어 쿼리 안에서 동적인 값을 업데이트하는식으로 사용해주면 훨씬 유지보수에 유리하고, 예측 가능하게 만들어줄수있어요 :)
/* Spacing */
--spacing-xs: 4px;
--spacing-sm: 8px;
--spacing-md: 16px;
--spacing-lg: 24px;
--spacing-xl: 32px;
--spacing-2xl: 40px;
/* Border Radius */
--radius-sm: 8px;
--radius-md: 16px;
--radius-lg: 40px;
--radius-full: 999px;
/* Font Sizes */
--text-sm: 14px;
--text-base: 16px;
--text-lg: 18px;
--text-xl: 20px;
--text-2xl: 24px;
--text-3xl: 40px;
/* Container Widths */
--container-sm: 640px;
--container-md: 1120px;
--container-lg: 1200px;| * { | ||
| box-sizing: border-box; | ||
| 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.css: CSS 초기화
- common.css: 전역 스타일
이렇게 용도에 따라 파일을 나눠 관리하는건 어떤 이점을 가져다줄수있는지 한번 더 생각해볼수있어요.
- 역할 분리가 되어 관리가 쉬움: 각 파일의 목적이 명확해서 수정이나 유지보수가 쉬워질거예요.
- 사용성과 확장성 증가: 여러 페이지 혹은 프로젝트에서 거의 동일하게 재사용이 가능해요.
- 스타일 우선순위가 더 예측 가능해짐: reset.css → common.css → 페이지나 컴포넌트별 스타일 순으로 로딩하면
우선순위가 명확해지고 스타일 충돌을 줄일 수 있어요.
| align-items: center; | ||
| } | ||
|
|
||
| .feature:nth-child(2) { |
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.
가상 선택자를 활용하는 시도는 좋지만,
가상 선택자보다는 직접 클래스명을 지어서 관리해주시는게 디자인 변경 및 유지보수에 유리할수있어요 :) 참고해주세요!
| <header> | ||
| <div class="header-contents"> | ||
| <div class="logo"> | ||
| <img src="images/panda_logo.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.
이미지 최적화도 조금 더 신경써볼까요?
웹페이지 성능 측정 지표중 CLS(Cumulative Layout Shift) 라는 지표가 있는데요,
현재 보고 있는 페이지에 갑자기 발생하는 레이아웃의 변경이 발생하면 사용자 입장에서 불편함을 겪을 수 있기때문에 레이아웃 이동(layout shift) 빈도를 측정해 성능을 평가한다고 보시면 됩니다. 보통 이미지 및 비디오 요소를 삽입할때 이런 현상이 자주 발생될수있겠죠?
따라서 레이아웃 시프트를 방지해주려면 이미지 및 비디오 요소에 width 와 height 속성을 항상 포함하거나 또는 CSS를 사용하여 필요한 공간(aspect-ratio-box)을 잡는 방법이 있습니다.
해당 이미지의 사이즈가 어느정도는 고정적이라면 width와 height를 직접적으로 넣는 방법이 있고, 반응형을 고려해 비율을 지키며 변해야한다면 aspect-ratio를 사용하는 방법도 있습니다.
여기에 쓰인 로고 이미지는 스크린사이즈 변화에 따라 줄어들거나 들어나지않을테니 고정 width와 height를 넣어주시는게 좋은 방법이 되겠죠?
이미지 최적화는 제가 제안드린 방법말고도 webp, avif 같은 차세대 확장자를 사용한다던지
srcSet을 사용해 각 스크린사이즈에 맞는 이미지파일을 로드한다던지
우선순위를 낮춰 레이지 로딩하는 등 여러가지 기법들이 있습니다.
차차 학습해보시면 좋을것같네요 👍
| <h1> | ||
| 믿을 수 있는<br> | ||
| 판다마켓 중고거래 | ||
| 판매를 원하는<br> | ||
| 상품을 등록하세요 | ||
| </h1> | ||
| <p> | ||
| 어떤 물건이든 판매하고 싶은 상품을<br> | ||
| 쉽게 등록하세요 | ||
| </p> |
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.
반응형을 대응한다고 생각했을때 이런 텍스트들을 음절별로 자동 줄바꿈해주고싶다면
word-break: keep-all; 속성을 사용하는 방식도 있답니다 :)
아래 아티클 참고해보세요!
참고
| <img src="images/instagram.png" alt="instargram" width="20"> | ||
| </a> | ||
| </div> | ||
| <section id="bottomBanner" class="banner"> |
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.
| </div> | ||
|
|
||
| <div id="footer-social"> | ||
| <a href="https://facebook.com" target="_blank"> |
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: 리퍼러 정보가 새 페이지로 전달되는 것을 방지
| <div class="header-contents"> | ||
| <div class="logo"> | ||
| <img src="images/panda_logo.png"> | ||
| <a href="/">판다마켓</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을 추가해볼까요?
| <img src="images/panda_logo.png"> | ||
| <a href="/">판다마켓</a> | ||
| </div> | ||
| <a id="loginLinkButton" class="button" href="login.html">로그인</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.
여기서도 네이밍케이스를 혼재해서 쓰셨네요!
|
|
||
| <main> | ||
| <section id="hero" class="banner"> | ||
| <div class="wrapper"> |
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.
wrapper가 너무 일반적인 네이밍처럼 느껴져요. 너무 일반적인 네이밍을 쓰다보면, 해당 이름이 프로젝트내에서 반복적으로 사용되어 유지보수 시 어려움을 가져다 줄 가능성이 있고, 나중에 다른 개발자가 동일한 클래스명으로 스타일을 또 만들면 스타일이 충돌되거나 꼬이는 문제가 생길 수도 있습니다.
이런 문제를 최대한 방지하기위해서는 최대한 클래스명을 신중히 고려해서 지어주시는게 좋습니다 👍
질문에 대한 답변
네! 디자인 의도대로 잘 구현해주시면됩니다 :) |
요구사항
기본
로그인 페이지
회원가입 페이지
심화
주요 변경사항
스크린샷
멘토에게