[임재은] Sprint1 #16
[임재은] Sprint1 #16
Conversation
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
여러가지 네이밍케이스가 혼재되어있고 포맷팅 실수가 조금 있는 편이네요.
습관으로 굳어지지않게 다음부터는 꼭 피드백 반영해봅시다 :)
주요 리뷰 포인트
- 네이밍 케이스
- 포맷팅
- 네이밍 컨벤션
index.html
Outdated
| @@ -0,0 +1,158 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
There was a problem hiding this comment.
언어 설정을 "ko"로 변경하시면, 한국어 사용자에 맞춰 스크린리더가 사용될수있고 검색엔진에서도 페이지의 언어를 한국어 콘텐츠로 인식하게끔 만들어줄 수 있습니다. 웹 접근성을 생각했을때 스크린리더를 사용한다면 lang="en" 상태에서는 한국어 콘텐츠를 영어 발음으로 읽을 가능성이 있기때문에, 한국어 사용자를 대상으로한다면 "ko"로 변경하시는게 옳습니다 :)
| <html lang="en"> | |
| <html lang="ko"> |
index.html
Outdated
| </nav> | ||
| </header> | ||
| <main> | ||
| <div class="kv_section"> |
There was a problem hiding this comment.
시맨틱 태그 사용을 늘려볼까요? main 아래 바로 섹션이면 div보다는 section을 사용하는게 좋을것같아요 :)
index.html
Outdated
| </div> | ||
| <div class="banner_section"> | ||
| <div class="banner_inner"> | ||
| <div class="card first"> |
There was a problem hiding this comment.
first와 같은 키워드를 사용하시는것보다는 엘리먼트의 역할에 따라 네이밍을 고려하시는게 좋아요.
지금은 과제라서 디자인이 변경될일은 없겠지만 변경된다면 이 이름을 다시 바꿔줘야겠죠? 이러한 네이밍이 늘어날수록 유지보수하기 애매해집니다 :)
index.html
Outdated
| <div class="banner_section"> | ||
| <div class="banner_inner"> | ||
| <div class="card first"> | ||
| <div class="img_wrap"> |
There was a problem hiding this comment.
여기에서는 언더바를 사용해주셨네요! 일관된 네이밍케이스를 지켜서 작성할수있도록 통일해줄까요?
아래 아티클 참고해보시면 좋을것같네요 :)
index.html
Outdated
| </p> | ||
| </div> | ||
| </div> | ||
| <div class="card second"> |
There was a problem hiding this comment.
요기에서도 위 코멘트 참고해서 네이밍 변경해주세요! :)
style.css
Outdated
| } | ||
| .kv_section { | ||
| background-color: #cfe5ff; | ||
| background-image: url(./images/kv.png); |
There was a problem hiding this comment.
background-image 속성을 사용해 이미지를 삽입하게되면 최적화 대응이 어려워집니다.
특별한 경우가 아니라면 img 태그를 사용해 이미지를 띄워줄까요?
style.css
Outdated
| padding-left: 2.96875rem; | ||
| } | ||
|
|
||
| .bottomBannerSection { |
There was a problem hiding this comment.
케밥케이스, 스네이크케이스, 카멜케이스가 혼재되어있네요...! 위에서 언급해드린 아티클 읽어보시고, 일관된 네이밍 케이스를 지켜주도록합시다 :)
style.css
Outdated
| .footer .footer_inner .footer-item:first-child { | ||
| grid-column: 2; | ||
| grid-row: 1; | ||
| text-align: left; | ||
| } | ||
| .footer .footer_inner .footer-item:nth-of-type(2) { | ||
| grid-column: 4; | ||
| grid-row: 1; | ||
| display: flex; | ||
| gap: 2rem; | ||
| } | ||
| .footer .footer_inner .footer-item:last-child { | ||
| grid-column: 6; | ||
| grid-row: 1; | ||
| display: flex; | ||
| gap: 1rem; | ||
| justify-content: flex-end; | ||
| } |
There was a problem hiding this comment.
first-child, nth-of-type등의 가상 클래스 선택자를 사용해보신건 좋지만, 너무 남용하면 유지보수에 좋지않아서 따로 클래스명을 지어주는게 나을것같네요 :)
style.css
Outdated
| @media (max-width: 1440px) { | ||
| html { | ||
| font-size: 15px; | ||
| } | ||
| } | ||
| @media (max-width: 1280px) { | ||
| html { | ||
| font-size: 13px; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 768px) { | ||
| html { | ||
| font-size: 12px; | ||
| } | ||
| } |
There was a problem hiding this comment.
요렇게 하시는것보다는 작은 화면에서부터 미디어 쿼리에 의한 재정의를 하는 구조로 바꿔보시는게 불필요한 스타일을 제거하는데 좀 더 유리합니다.
그리고 미디어 쿼리를 사용하실때 공통으로 사용되는 값들을 css 변수로 정의한다음 각 브레이크 포인트에서 변수값만 변경해주는식으로 활용하시면 스타일도 중복 제거하면서 유지보수에도 용이해집니다.
코멘트 참고해보시고 리팩토링해보세요! :)
test.txt
Outdated
질문에 대한 답변웹접근성에 맞게 코드가 진행되었는지 궁금합니다표준화된 마크업을 하셨는지 체크할수있는 서비스가 있습니다 :) 배포 url을 제출해서 확인해보시고, 개선 항목을 발견해보세요 :) 또한 웹 접근성은 시맨틱 태그 사용, aria, 명도대비등 고려할 요소가 많아서 웹 접근성을 잘 지킨 웹사이트나 좋은 사례를 찾아보시면 도움이 많이 될것같아요! 심화조건에 부합하는지 궁금합니다 !rem을 사용하고 default font size를 활용하면 rem을 적용한 요소들이 스케일 줄이듯이 같이 작아질거예요. |
질문에 대한 답변안녕하세요 멘토님. 4번의 요구사항에 대해 질문이 있습니다.브라우저의 넓이 크기가 작아질 경우 상단 로고와 로그인 버튼 여백이 너무 큰 거 같아서 브라우저 창의 크기가 작아지면 여백 값도 변경되게 CSS를 추가 했는데 우선 기본 요구사항은 언제든 양옆 200px 여백을 유지하라는 요구사항이라서 요구사항에는 안맞는 구현이긴해요. |
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게