[최창환] Sprint1 #4
Conversation
addiescode-sj
left a comment
addiescode-sj
left a comment
There was a problem hiding this comment.
수고하셨습니다!
주요 리뷰 포인트
- 일관된 기준으로 클래스이름 적용
- 시맨틱 태그 사용
- CSS 변수 추출
| <main> | ||
| <div class="link"> | ||
| <div class="link-img"> | ||
| <div class="link-img-text"> |
There was a problem hiding this comment.
전체적으로 css 클래스 네이밍이 일관적이지 않게 쓰인 느낌이 드네요 :)
여러가지 방식이 있을 수 있겠지만, 네이밍 컨벤션중에 가장 일반적으로 사용되는 BEM도 참고해보시면 좋을것같아요!
| ><a href="/items"><img src="img/btn_large.png" alt="btn" /></a> | ||
| </div> | ||
|
|
||
| <img class="link-img-panda" src="img/Img_home_top.png" alt="" /> |
There was a problem hiding this comment.
귀찮더라도 img태그에는 꼭 alt를 사용해주세요! :)
웹 접근성뿐만 아니라, 이미지 로딩 실패 시 대체 텍스트를 표시하거나 검색엔진최적화에도 도움을 줄 수 있기 때문에 꼭 사용해주시는게 좋습니다.
레퍼런스로 애플 공식 웹사이트에 가서 alt텍스트를 어떻게 사용했는지 참고해보시면:
<img src="/kr/macbook-air/images/overview/design/design_hero_static__e56c1v71mr6u_large.jpg" onload="__lp(event)" alt="열려있는 MacBook Air 13 및 15의 모습. 한 대에는 디자인 작업을 진행 중인 화면이, 다른 한 대에는 이메일과 스프레드시트를 넘나들며 멀티태스킹을 하는 화면이 표시되어 있습니다">이런식으로 alt 속성에 이미지 설명을 위해 구체적이고 명확한 설명을 제공하는 모습을 확인해보실 수 있습니다 :)
| <div class="content"> | ||
| <div class="content-main"> | ||
| <div class="content-box"> |
There was a problem hiding this comment.
만약 BEM을 적용한다면,
content => main-content
content-main => main-content__item
content-box => main-content__body
이정도가 될 수 있겠네요! 어떤가요?
| </div> | ||
| <div class="content-main"> | ||
| <div class="content-box"> | ||
| <div class="content-text reverse"> |
There was a problem hiding this comment.
불필요한 div 중첩을 피하고 reverse 클래스를 추가하는 형식으로 잘 작업해주셨네요 👍
| </div> | ||
| </div> | ||
| </main> | ||
| <div class="footer"> |
There was a problem hiding this comment.
시맨틱 태그의 활용도를 높여보시면 좋을것 같아요! :)
<footer></footer>
| </div> | ||
| </header> | ||
| <main> | ||
| <div class="link"> |
| @@ -0,0 +1,191 @@ | |||
| @import "reset.css"; | |||
There was a problem hiding this comment.
css파일을 용도에 맞게 잘 구분해서 사용하셨네요 :) 굳굳!
| height: 70; | ||
| gap: 10px; | ||
| padding-top: 9px; | ||
|
|
There was a problem hiding this comment.
중간에 이런 공백이 혹시 왜 필요한걸까요? 다음부터는 포맷팅도 신경써볼까요? :)
| align-items: end; | ||
| width: 100%; | ||
| height: 540px; | ||
| background-color: rgba(207, 229, 255, 1); |
There was a problem hiding this comment.
이렇게 반복되는 값들은 코드 중복을 없애고 재사용성을 높이기위해 추출해서 css 변수로 사용하는것을 추천드려요!
:root {
--primary-color: rgba(54, 146, 255, 1);
--text-color: rgba(55, 65, 81, 1);
--background-color: rgba(255, 255, 255, 1);
--footer-bg: rgba(17, 24, 39, 1);
--footer-text: rgba(156, 163, 175, 1);
--content-bg: rgba(252, 252, 252, 1);
--link-bg: rgba(207, 229, 255, 1);
--container-width: 1120px;
--content-width: 988px;
--header-height: 70px;
--footer-height: 160px;
--spacing-xs: 9px;
--spacing-sm: 12px;
--spacing-md: 24px;
--spacing-lg: 32px;
--spacing-xl: 64px;
--font-size-sm: 16px;
--font-size-md: 18px;
--font-size-lg: 24px;
--font-size-xl: 40px;
}
질문에 대한 답변div 배치나 class명도 부정확한것 같고, css 속성을 너무 중복해서 많이 사용한듯 합니다...네, 우선 전체적으로 엘리먼트간 위계가 보이지않아 일관적이지않게 html css를 작성하시게된것같아요 :) 사용하는 화면이 1920px보다 작아서 화면을 축소한 상태로 작업을 해서 요구사항을 잘 지킨건지 모르겠습니다..화면 테스트 해보면서 몇가지 개선이 필요해보이는 이슈 골라드릴게요!
|
og image를 png에서 jpg로 변경
2 participants
요구사항
기본
심화
스크린샷
멘토에게