-
Notifications
You must be signed in to change notification settings - Fork 39
[명지우] sprint1 #23
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 #23
The head ref may contain hidden characters: "Basic-\uBA85\uC9C0\uC6B0-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 단위 쪼개서 형식을 정해 해주신 것 좋습니다.
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.
👍
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Noto+Sans+KR:wght@400;700&family=Poppins:wght@400;700&display=swap" | ||
| rel="stylesheet" | ||
| /> |
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.
❗️ 수정요청
제가 알기로는 디자인에서 Pretendard 만 사용하고 notoSans는 사용하지 않는 걸로 알아요~
사용하지 않는 폰트라면 제거해주시고 Pretendard를 사용해주세요~
| <link rel="stylesheet" href="styles/base.css" /> | ||
| <link rel="stylesheet" href="styles/header.css" /> | ||
| <link rel="stylesheet" href="styles/banner.css" /> | ||
| <link rel="stylesheet" href="styles/section.css" /> | ||
| <link rel="stylesheet" href="styles/footer.css" /> | ||
| <!-- 반응형 : 타블렛, 핸드폰 지원 --> | ||
| <link rel="stylesheet" href="styles/tablet.css" /> | ||
| <link rel="stylesheet" href="styles/phone.css" /> |
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를 목적별로 분리해주신 점 좋습니다~
다만 이렇게 되면 해당 파일에서 불러와야 하는 css 파일이 너무 많아지니 아래와 같은 방식을 추천드려요.
| <link rel="stylesheet" href="styles/base.css" /> | |
| <link rel="stylesheet" href="styles/header.css" /> | |
| <link rel="stylesheet" href="styles/banner.css" /> | |
| <link rel="stylesheet" href="styles/section.css" /> | |
| <link rel="stylesheet" href="styles/footer.css" /> | |
| <!-- 반응형 : 타블렛, 핸드폰 지원 --> | |
| <link rel="stylesheet" href="styles/tablet.css" /> | |
| <link rel="stylesheet" href="styles/phone.css" /> | |
| <link rel="stylesheet" href="styles/base.css" /> | |
| <link rel="stylesheet" href="styles/index.css" /> |
페이지에 대응하는 css 파일을 만들고 내부적으로 해당 페이지 구현을 위해 필요한 css 들을 import하는 방식입니다.
이렇게 하시면 index.html 은 필요한 파일만 가지게 되고 관리측면에서도 편리합니다.
| <body> | ||
| <header> | ||
| <a href="/"> | ||
| <img src="images/Img_logo.png" alt="로고 이미지" class="image-logo" /> |
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.
💊 제안
img 태그의 alt 속성은 alternative 라는 의미로, 이미지 파일을 다운로드하는 것에 실패해서 이미지 파일을 보여줄 수 없을 때
어떤 이미지인지 파악할 수 있게 대신 제공되거나, 스크린리더로 읽혀지는 문자를 의미합니다.
따라서 가능한 해당 이미지를 설명하는 내용을 넣어주면 좋습니다~
지금의 경우 해당 이미지가 안 보일때 제공되면 좋을 텍스트는 이미지에 들어간 문자인 "판다마켓"이 되겠네요~
| <img src="images/Img_logo.png" alt="로고 이미지" class="image-logo" /> | |
| <img src="images/Img_logo.png" alt="판다마켓" class="image-logo" /> |
| <div class="footer-sns"> | ||
| <a href="https://www.facebook.com/" target="_blank"> | ||
| <img src="images/Ic_facebook.png" alt="페이스북 아이콘" | ||
| /></a> | ||
| <a href="https://x.com/" target="_blank"> | ||
| <img src="images/Ic_twitter.png" alt="트위터 아이콘" | ||
| /></a> | ||
| <a href="https://www.youtube.com/" target="_blank"> | ||
| <img src="images/Ic_youtube.png" alt="유튜브 아이콘" | ||
| /></a> | ||
| <a href="https://www.instagram.com/" target="_blank"> | ||
| <img src="images/Ic_instagram.png" alt="인스타그램 아이콘" | ||
| /></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="footer-sns">
<li>
<a href="https://www.facebook.com/" target="_blank">
<img src="images/Ic_facebook.png" alt="페이스북" />
</a>
</li>
</ul>| width: 100%; | ||
| padding-top: 143px; | ||
|
|
||
| display: flex; | ||
| justify-content: center; | ||
| align-items: flex-end; | ||
| gap: 69px; | ||
|
|
||
| background-color: #cfe5ff; |
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 속성들을 공백으로 구분하신걸까요?
어떤 기준이 있으신지 궁금합니다~
만약 기준이 없다면 공백이 없는게 가독성 측면에서 더 좋을 것 같아요~
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, display..)
- 테두리 및 라운드
- 세부 스타일링 (배경 색, 폰트 색상 및 크기...)
- 애니메이션 및 커서
이런 기준으로 나눠서 작성하고 있습니다!
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 코드가 길어질 때 관리하는 다른 방법이 있을까요??
| display: flex; | ||
| justify-content: center; | ||
| align-items: flex-end; | ||
| gap: 69px; |
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.
💊 제안
제가 봤을때 gap 속성은 없어도 될 것 같아요~
디자인 구현에 불필요한 속성들이 있으면 유지보수나 수정시 헷갈리니 이러한 속성들은 지워주시는 것을 추천드려요!
(만약 피그마를 따르셨다면, 피그마는 디자인 파일 기반으로 css 를 생성하기 때문에 특이한 값들을 자주 준답니다!)
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; |
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.
💊 제안
이 속성이 필요한지 모르겟습니다.
없어도 동일동작할 것 같아요~
| @media screen and (max-width: 767px) { | ||
| /* header */ | ||
| header { | ||
| padding: 9.5px 72px; |
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에서의 소수점 사용에 대해 다양한 의견이 있습니다.
예전에는 이러한 소수점이 정수로 변환된다는 것이 정론에 가까웠으나, 다양한 해상도가 나오면서 이러한 소수점도 반영된다는 의견이 있습니다.
정확한 명세로 나온것은 없지만, 저는 소수점 사용을 지양하는 편입니다.
만약 같은 화면 크기임에도 해상도에 따라 소수점이 반영되는 것이 갈린다면, 디자인의 일관성이 떨어지는 것이고 소수점 사용은 가독성에 좋지 못하다고 생각하기 때문입니다~
이와 관련해 다양한 논의들이 있으니 참고해보세요!
https://www.reddit.com/r/css/comments/148y3xm/is_it_ok_to_give_text_the_size_in_decimals_like/
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 를 분리해두셨지만 media query 문은 분기별 파일을 만드셨군요~
만약 쪼개실거라면, media query 문도 해당하는 파일 하단-footer 관련 미디어 쿼리문은 footer.css 하단, header 관련 미디어 쿼리문은 header.css 하단으로-으로 옮기시는 것이 더 좋지 않을까합니다.
추후 페이지가 늘어나고 현재 구조를 따르면 각 페이지의 반응형 css 들이 한 파일에 작성될텐데 그렇게 분리한 것의 장점을 모르겠어서요~
만약 이렇게 하고 싶으시다면 페이지별 tablet, phone 용 css 파일을 만드시는 것을 제안드려요~
요구사항
기본
🌐 https://myungjiwoo-panda-market.netlify.app/
심화
스크린샷
멘토에게