-
Notifications
You must be signed in to change notification settings - Fork 39
[유서현] sprint1 #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[유서현] sprint1 #22
The head ref may contain hidden characters: "Basic-\uC720\uC11C\uD604-sprint1"
Conversation
GANGYIKIM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서현님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
다음 주차도 화이팅입니다~
- commit 단위를 쪼개시고 형식에 따라 commit 하신점 좋습니다.
- css랑 html 파일내에서 구조화하려고 하신 점 좋습니다!
- sprint1 폴더를 만드셔서 거기서 작업을 하셨는데, sprint2가 시작하면 어떻게 하실 예정이실까요? sprint[구현주차수] 로 폴더명을 변경하시면서 작업하시는 것은 비추드립니다. 그렇게되면 git 자체적으로 해당 폴더 전체가 변경되었다고 판단하고 제가 변경사항에 대해 파악해 리뷰드리기가 어려워집니다~ 따라서 가능하면 sprint1로 폴더명을 유지하시는 것을 추천드리고, 신경쓰이신다면 sprint1 폴더를 지우시고 나머지 파일들을 root로 빼주시는 것을 추천드려요~
| /* position: sticky; | ||
| top: 0px; */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
주석은 히스토리상 필요하거나 코드설명이 필요한 경우처럼 필요한 경우만 남겨두는 것이 좋습니다.
만약 작업중 확인하시려고 넣으신거면 삭제해주세요~
| /* position: sticky; | |
| top: 0px; */ |
| background-color: #1251AA; | ||
| } | ||
|
|
||
| /* 메인 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
파악하기 쉽게 관련된 css 들을 모아두시고 주석으로 구분해주신점 좋습니다!
| text-align: center; | ||
| align-items: center; | ||
| justify-content: center; | ||
| text-decoration: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
브라우저 마다 자체적으로 스타일을 가지고 있습니다.
위의 코드에서 a 태그의 text-decoration: none을 적용해주신건, 이미 text-decoration: underline 속성을 가지고 있기 때문이죠~
이런식으로 이미 가지고 있는 스타일을 없애 원하는 스타일을 적용하기 위한 작업이 필요합니다.
주로 이러한 역할을 하는 css 파일을 reset.css 혹은 normalize.css 라는 이름으로 만들어서 관리합니다.
서현님께서는 지금 필요한 태그의 스타일만 초기화하고 사용하셨지만 reset.css 같은 파일을 만드셔서
다른 태그들의 기본 스타일도 초기화하고 작업하시는 것이 예상치 못한 동작도 방지하고 코드 반복을 줄일 수 있어 추천드려요!
혹시 어떤 내용들이 들어가는지 궁금하시다면 reset.css, normalize.css 를 구글에 검색해보시면 다양한 파일이 나오니
확인해보시고 마음에 드는 것을 참고하시면 좋을 것 같습니다.
| <meta name="description" content="판다마켓" /> | ||
| <meta | ||
| name="keywords" | ||
| content="판다마켓, 중고거래, 중고상품, 판매, 구매, 거래" | ||
| /> | ||
| <meta name="language" content="ko" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
메타태그 작성하신 것 좋습니다 👍
| </div> | ||
| <!-- 버튼 --> | ||
| <div class="nav"> | ||
| <!-- 로그인 버튼 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
주석을 다는 것은 좋은 습관이지만, 가장 좋은 코드는 주석 없이도 이해할 수 있는 코드입니다.
현재 코드 구조상 주석 없이도 충분히 이해할 수 있어 보이는데, 오히려 주석이 많아 가독성을 해칠 수 있을 것 같아요.
필요한 부분에만 간결하게 주석을 추가하면 더 좋은 코드가 될 것 같습니다!
| <!-- 버튼 --> | ||
| <div class="nav"> | ||
| <!-- 로그인 버튼 --> | ||
| <a class="login-btn" href="/login.html" button>로그인</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ 질문
앵커 태그에서 button 속성을 주셨는데, 어떤 용도로 사용하시는 건지 궁금합니다~
mdn에 안나와있는데, custom-attribute로 사용하신걸까요?
만약 불필요한 속성이라면 지우시는 것을 추천드려요~
| <div class="icons"> | ||
| <a href="https://www.facebook.com" target="_blank"> | ||
| <i class="fa-brands fa-facebook"></i> | ||
| </a> | ||
| <a href="https://www.twitter.com" target="_blank"> | ||
| <i class="fa-brands fa-twitter"></i> | ||
| </a> | ||
| <a href="https://www.youtube.com" target="_blank"> | ||
| <i class="fa-brands fa-youtube"></i> | ||
| </a> | ||
| <a href="https://www.instagram.com" target="_blank"> | ||
| <i class="fa-brands fa-instagram"></i> | ||
| </a> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
이 영역은 로고들의 모음이라고 할 수 있습니다. 관련된 요소들끼리 묶인 목록이니 의미록적인 마크업을 작성한다면 list 관련 태그로 작성하시는 것을 추천드립니다.
<ul class="icons">
<li>
<a href="https://www.facebook.com" target="_blank">
<i class="fa-brands fa-facebook"></i>
</a>
</li>
</ul>
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게