KDT5 Chadongmin Clone Coding PR #33
Conversation
|
li태그 안에 ul이 들어갔는데 nav > ul > li 구조가 시멘틱한 마크업에 해당합니다. aria-label을 이용하는거를 더 추천드립니다! |
|
저 스스로도 li 하위요소로 ul을 쓰면서 읽기 굉장히 힘들다고 생각했는데 다음엔 참고해서 시맨틱한 마크업을 사용할 수 있도록 하겠습니다 aria-label이라는 개념을 처음 알게 되었는데 시각장애인용 음성지원을 할 수 있는 속성이라고 하니 button 요소 안에 span 등을 넣거나 title 속성을 쓰는 대신 aria-label을 쓰겠습니다! 홈페이지 레이아웃상 h1태그로 주 제목을 짓기 애매하다고 판단해 넣지 않았는데 |
sihyun92
left a comment
sihyun92
left a comment
There was a problem hiding this comment.
멘토님이 말씀하신 것처럼 BEM을 너무 잘 쓰셔서 진짜 배울 점이 많은 코드라고 생각합니다~ 저도 동민님 코드 보고 제 프로젝트에 문제점을 보완하는데 아주 중요한 코드라고 생각이 들어요^^ 다만 시멘틱한 코드를 썼다면 좀 더 알아보기 쉬운 코드가 아닐까 하는 아쉬운 점이 있었습니다. 고생 많으셨어요^^
There was a problem hiding this comment.
주석이 없어서 어떤 의도로 만든 코드인지 불분명하네요 이 코드를 읽기 위해서는 html 파일까지 왔다갔다할 수 있어서 동민님이 수정하실때 왔다갔다 여러번 해야할 수도 있어요 ㅜㅜ 그리고 하나의 팁인데 addEventListner 두번째 파라미터에 함수를 넣어주셨는데 이런 방식도 괜찮고, 함수를 따로 나눠서 함수명만 넣어줘도 가독성 확실히 많이 올라갈거에요 ㅎㅎ
There was a problem hiding this comment.
주석으로 코드의 목적이나 구간 명시를 해두는게 얼마나 코드리딩에 도움이 되는지 이번 코드리뷰로 깨닫게 되네요!
이벤트리스너로 들어가는 콜백함수를 따로 명시하고 함수명으로 넣어주기 명심하겠습니다~!
| <script defer src="./js/nav-bar.js"></script> | ||
| </head> | ||
| <body> | ||
| <main> |
There was a problem hiding this comment.
코드를 보니까 이게 header 영역인거 같은데 header라고 명시해주는게 더 좋았을 뻔했는데 아쉽습니다 ㅜㅜ main 태그는 주 content 영역에 쓰인답니다 ^^
There was a problem hiding this comment.
저도 한참 만들고나서 main태그를 잘못 썼다는걸 인지했습니다만 싹 다 뜯어 고치기가 겁나서 그대로 사용했습니다 ㅠㅠ 개인 프로젝트라 망정이지 협업이었으면 민폐였겠다 싶어 이번 기회에 반성하고 시멘틱 태그의 올바른 사용을 할 수 있도록 하겠습니다!
There was a problem hiding this comment.
코드를 전체적으로 한번 줄여보는 것도 도움이 많이 되실겁니다. 코드가 복잡한거 치고는 알아보기는 쉽습니다 ^^ div 태그를 사용하실때는 class 이름으로 명시해준다면 완성도 높은 코드라고 생각듭니다 ㅎㅎ
There was a problem hiding this comment.
제가 상대적으로 긴 코드 대비 클래스를 잘 안쓰고 있더라구요 ㅜㅜ 앞으로는 지금보다 효율적으로 코드를 짤 수 있도록 해보겠습니다!
| </div> | ||
| </div> | ||
| <div class="section__mid"> | ||
| <div class="boxoffice__block"> |
There was a problem hiding this comment.
BEM을 정말 활용 잘하시네요 ㅎㅎ 저도 이 부분을 잘 못하는데 동민님 코드 보고 하나 더 배워갑니다 ㅎㅎ 대단하세요 ^^
There was a problem hiding this comment.
감사합니다~~ 클래스 이름 짓는게 개발자들 골머리 썩이는 일이라고 듣기만 했는데 이번 기회에 새삼 느끼게 되었습니다 ㅎㅎㅎ
kledyu
left a comment
kledyu
left a comment
There was a problem hiding this comment.
저는 CSS나 JS파일을 모듈화하지도 못했는데 동민님은 모듈화도 하셔서, 유지보수에도 용이하게끔 코드를 관리하셨다는 생각이 듭니다! 시맨틱 태그도 최대한으로 활용하신 것은 배워갑니다! 항상 동민님 수고가 많으십니다! 화이팅!!
| </div> | ||
| </section> | ||
| </main> | ||
| <aside> |
There was a problem hiding this comment.
aside 시맨틱 태그 사용은 정말 탁월한 것 같습니다! 최대한 시맨틱 태그를 활용하신 모습 배워갑니다!
There was a problem hiding this comment.
감사합니다~~~ aside 태그의 본래 취지에 맞게 쓴건지는 잘 모르겠지만 그냥 div로 묶는것보단 나을 것 같아 써보게 됐습니다!
| infoBanner.forEach(function(event) { | ||
| event.classList.remove("banner--moved") | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
forEach 메소드의 활용으로 infoBanner 요소에 class를 추가하는 동작을 구현하신 것을 보니 저보다 JS 메소드 활용이 더 능숙하신 것 같습니다! 배워갑니다 :)
There was a problem hiding this comment.
이 두줄 쓰려고 몇 시간 동안 구글링에 매달렸던 것 같습니다 ㅎㅎㅎ... 나중엔 눈감고도 뚝딱뚝딱 쓰게 되면 좋겠네요 XD
| <div class="inner"> | ||
| <div class="logo"> | ||
| <a href="/"> | ||
| <img src="./images/logo-white_new2.png" alt="" /> |
There was a problem hiding this comment.
사실 저도 미흡하고 부족한 부분인데 사진이 렌더링 되지 못했을 때를 대비하여 alt에 이미지를 설명하는 문구가 있었으면 좋았을 듯 합니다!
There was a problem hiding this comment.
이부분 자주 잊고 넘어가게 되더라구요 ㅜㅜ img파일의 alt요소 꼭 신경쓰기!
4 participants
차동민 메가박스 클론코딩 과제 제출합니다!
원본 사이트 : https://www.megabox.co.kr/
클론 사이트 : https://kdt5-chadongmin--kdt5-cdm-megaboxclone.netlify.app/