[이찬호] Sprint1 #37
[이찬호] Sprint1 #37
Conversation
GANGYIKIM
left a comment
GANGYIKIM
left a comment
There was a problem hiding this comment.
찬호님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
다음 주차도 화이팅입니다~
- git commit 단위를 나누시는 연습을 해보시면 좋을 것 같아요. index page css 추가, html 수정과 같은 식으로 작게 쪼개시면 시점을 돌리시기도 더 좋답니다.
- 배포된 사이트 기준으로 footer height가 너무 작고, header의 height가 너무 큰 것 등 디자인과 다른 요소들이 있는 것 같아요. 가능하면 디자인과 동일하게 작업해주세요~
There was a problem hiding this comment.
❗️ 수정요청
해당 이미지 파일의 크기를 보면 588× 444로 되어 있습니다.
저희는 반응형 사이트를 만들고 있고, 이런 이미지들은 모바일에서도 사용할 것이기 때문에 추출하실때, 1배수가 아닌 2배수로 추출해주시는 것이 화질면에서 좋습니다.
아래처럼 figma export 탭에서 2x 로 설정하고 추출해주세요~
There was a problem hiding this comment.
💊 제안
한 레포안에서는 동일한 룰을 따르는 것이 좋습니다.
따라서 img_home_01.png와 같은 식으로 같은 네이밍 룰을 따르시는 것을 추천드려요~
There was a problem hiding this comment.
❗️ 수정요청
해당 파일명에 공백이 들어가 있는데 이는 예기치 않은 동작을 발생시킬 수 있습니다. 이러한 공백은 자동으로 공백을 의미하는 대체 문자열로 변환되기 때문입니다.
그래서 일반적으로 공백대신 _ -를 이용해 icon_img, icon-img 같은 식으로 공백을 표현합니다.
해당 이미지의 경우 logo이니 간단하게 logo.png로 변경하시는 것을 추천드려요
https://developer.mozilla.org/ko/docs/Glossary/Percent-encoding?utm_source=chatgpt.com
There was a problem hiding this comment.
❗️ 수정요청
지금 이미지 파일이 크기별로 존재하고 사용하시는 파일은 public/logos/Property 1=lg.png 하나이니, 사용하지 않는 파일은 지우시는 것을 추천드립니다~
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| body { | ||
| font-family: "Pretendard", system-ui, -apple-system, BlinkMacSystemFont, | ||
| "Segoe UI", Roboto, Oxygen, Ubuntu, Cantarell, "Open Sans", "Helvetica Neue", | ||
| sans-serif; | ||
| color: #374151; | ||
| } | ||
|
|
||
| a { | ||
| text-decoration: none; | ||
| } | ||
|
|
||
| img { | ||
| max-width: 100%; | ||
| height: auto; | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
💊 제안
간략한 reset css 속성들이라 따로 분리하지 않으시고 한 파일에 작성하셨나봐요~
하지만 필요시 특정 태그의 스타일을 초기화하는건 번거로운 일이고, 이를 염두에 두고 css 작성을 해야하니 가능하면 처음부터 브라우저 스타일을 초기화하시고 reset.css로 분리하시고 작업하시는 것을 추천드립니다!
+또한 관심사 분리 측면에서도 css의 목적별로 분리하시는 것이 더 좋습니다.
There was a problem hiding this comment.
❗️ 수정 요청
스크린샷을 코드에 추가하신 이유가 PR에 첨부하려는 목적 때문인 것 같아요~
PR에 이미지를 첨부하려면 해당 repo에 올려서 사용하실 수도 있지만, 이는 오래 유지되어야 하는 이미지에 적합한 방식입니다.
PR에 올리는 이미지는 일시적인 참고용이므로, 굳이 코드에 추가하지 않고 drag & drop으로 첨부하는 것을 추천드립니다!
| <!-- 메인 --> | ||
| <main> |
There was a problem hiding this comment.
💊 제안
주석을 다는 것은 좋은 습관이지만, 가장 좋은 코드는 주석 없이도 이해할 수 있는 코드입니다.
현재 코드 구조상 주석 없이도 충분히 이해할 수 있어 보이는데, 오히려 주석이 추가적인 정보를 전달하지 못하고 가독성을 해치고 있는 것 같아요.
필요한 부분에만 주석을 추가하면 더 좋은 코드가 될 것 같습니다!
| <div class="social-links"> | ||
| <a href="https://facebook.com" target="_blank" class="social-icon"> | ||
| <img src="public/icons/ic_facebook.png" alt="페이스북" /> | ||
| </a> | ||
| <a href="https://x.com" target="_blank" class="social-icon"> | ||
| <img src="public/icons/ic_twitter.png" alt="트위터" /> | ||
| </a> | ||
| <a href="https://youtube.com" target="_blank" class="social-icon"> | ||
| <img src="public/icons/ic_youtube.png" alt="유튜브" /> | ||
| </a> | ||
| <a href="https://instagram.com" target="_blank" class="social-icon"> | ||
| <img src="public/icons/ic_instagram.png" alt="인스타" /> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
💊 제안
이 영역은 로고들의 모음이라고 할 수 있습니다. 관련된 요소들끼리 묶인 목록이니 의미록적인 마크업을 작성한다면 list 관련 태그로 작성하시는 것을 추천드립니다.
<ul class="social-links">
<li>
<a href="https://facebook.com" target="_blank" class="social-icon">
<img src="public/icons/ic_facebook.png" alt="페이스북" />
</a>
</li>
</ul>There was a problem hiding this comment.
👍 칭찬
HTML 을 이해하기 쉽게 잘 작성하셨어요!
| <section class="register"> | ||
| <div class="register-container"> | ||
| <div class="register-image"> |
There was a problem hiding this comment.
💊 제안
섹션 2 ~ 4까지를 각각의 classname을 이용해 구분하신 점 좋습니다.
다만 섹션 2 ~ 4 까지가 반복되는 요소가 많은 영역이니 반복을 줄일 수 있는 방법을 고민해보시면 좋을 것 같아요~
아래처럼 공통으로 사용하는 class를 정의해두시고 구분을 위한 class들을 추가해주셔도 좋을 것 같아요!
// section 태그별 공통되는 스타일이 정의된 main-list-container 추가로 반복되는 css 사용을 줄이고 유지보수하기 쉽게 바꿈
<section class="main-list-container hot-item" /> // section2
<section class="main-list-container search" /> // section3
<section class="main-list-container register" /> // section4
2 participants
요구사항
기본
심화
주요 변경사항
스크린샷
![image]
멘토에게