KDT5 이은지 1차과제 리팩토링-2 #82
Conversation
index.html
Outdated
| <body> | ||
| <!-- HEADER --> | ||
| <header> | ||
| <div class="header__main"> |
There was a problem hiding this comment.
로그인, 회원가입 등의 메뉴가 있는 부분을 header__main, 영화, 극장, 예매 등의 메뉴가 있는 부분을 header__nav-line으로 class명을 붙여주셨는데 <div>.header__main => <nav>.sub-menu, <div>.header__nav-line => <nav>.main-menu로 semantic tag를 사용하면서 class명을 조금 더 직관적으로 바꿔 보시는 건 어떨까요? 각 섹션별로 계층구조를 나누면서 class명을 붙이기가 어려우셨던 것 같아서 container 역할을 하는 태그를 비교적 심플하게 짓고 하위 요소들을 세부적으로 나눠보시는 것도 좋을 것 같습니다:)!
There was a problem hiding this comment.
넵넵 header__nav-line은 구조를 다 잡고나서 나중에 급하게 감싼 태그명이라 생각한대로 이름이 안지어지긴 했습니다ㅠㅠ nav태그를 좀더 활용해볼수 있었겠네요! 시멘틱 태그를 header, section 정도로만 사용한것 같아 기억해뒀다가 다른태그도 활용해보겠습니다!
index.html
Outdated
| <div class="title-chart chart-strong">무비차트</div> | ||
| <div class="title-schedule">상영예정작</div> | ||
| </div> | ||
| <div class="swiper__all-btn"> |
There was a problem hiding this comment.
class명을 allbtn => all-Btn으로 수정해주시면 될 것 같습니다!
index.html 문서를 기준으로 197-199, 728-730, 810-812 라인입니다:)
index.html
Outdated
| <span class="header__logo-text">CULTUREPLEX</span> | ||
| </a> | ||
| </div> | ||
| <ul class="header__my-Menu"> |
There was a problem hiding this comment.
class명을 작성하실때 header__my-Menu,all-Btn 에서 케밥 케이스를 사용하신것 같습니다! 혹시 이때 소문자가 아닌 대문자로 작성하신 이유가 있을까요? 만약 케밥케이스를 적용하신 부분이라면 소문자로 작성하는것이 더 적합한 표기법이라고 생각됩니다! nav-line이나 item-title등에서는 소문자를 사용하고 있으셔서 따로 이유가 있으신지 궁금합니다! 🥸
There was a problem hiding this comment.
두개 단어를 이어붙일때 클래스명은 대시로 구분하여 all-btn, my-menu 로 하고 js에서 변수명을 지을때는 allBtn, myMenu이런식으로 했는데.. 두가지가 섞인 혼종이 탄생해 버렸습니다ㅠㅠ 제가봐도 통일성 없이 지은거같아 다음 프로젝트때는 클래스명에 유의해서 작업하려고 합니다!
index.html
Outdated
| </li> | ||
| </ul> | ||
| </div> | ||
| <div class="header__nav-line"> |
There was a problem hiding this comment.
my-menu 부분까지는
태그, header__nav-line 부분은 sementic tag 태그를 활용해본다면 어떨까요? 태그를 활용하면 코드가 더욱 간결화 될 것 같습니다! :)There was a problem hiding this comment.
넵넵 시멘틱 태그를 활용하면 각 역할이 좀더 명확하게 드러나고 간결화 될 것 같습니다! 피드백 감사드려요~
|
리팩토링 진행하시느라 정말 고생 많으셨습니다! 저는 JS부분은 아직 미숙해서 함께 공부했던 class명 sementic teg에 관련해 리뷰를 남겼습니다! |
index.html
Outdated
| <body> | ||
| <!-- HEADER --> | ||
| <header> | ||
| <div class="header__main"> |
There was a problem hiding this comment.
넵넵 header__nav-line은 구조를 다 잡고나서 나중에 급하게 감싼 태그명이라 생각한대로 이름이 안지어지긴 했습니다ㅠㅠ nav태그를 좀더 활용해볼수 있었겠네요! 시멘틱 태그를 header, section 정도로만 사용한것 같아 기억해뒀다가 다른태그도 활용해보겠습니다!
index.html
Outdated
| <span class="header__logo-text">CULTUREPLEX</span> | ||
| </a> | ||
| </div> | ||
| <ul class="header__my-Menu"> |
There was a problem hiding this comment.
두개 단어를 이어붙일때 클래스명은 대시로 구분하여 all-btn, my-menu 로 하고 js에서 변수명을 지을때는 allBtn, myMenu이런식으로 했는데.. 두가지가 섞인 혼종이 탄생해 버렸습니다ㅠㅠ 제가봐도 통일성 없이 지은거같아 다음 프로젝트때는 클래스명에 유의해서 작업하려고 합니다!
index.html
Outdated
| </li> | ||
| </ul> | ||
| </div> | ||
| <div class="header__nav-line"> |
There was a problem hiding this comment.
넵넵 시멘틱 태그를 활용하면 각 역할이 좀더 명확하게 드러나고 간결화 될 것 같습니다! 피드백 감사드려요~
index.html
Outdated
| <div class="title-chart chart-strong">무비차트</div> | ||
| <div class="title-schedule">상영예정작</div> | ||
| </div> | ||
| <div class="swiper__all-btn"> |
멘토님의 코드리뷰를 반영한 리팩토링 Pull Request
CGV 홈페이지 클론코딩
과제 결과물 : https://dmswl2030.github.io/Clone-CGV/
원본 사이트 : https://www.cgv.co.kr/#none
[리팩토링 반영 내용 - 클래스명 수정]