-
Notifications
You must be signed in to change notification settings - Fork 79
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 #2
The head ref may contain hidden characters: "part1-\uCC9C\uAD8C\uD76C-sprint1"
[천권희] Sprint1 #2
Conversation
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 본문 상세화
아래와 같이 PR 본문 템플릿에 몇 가지 추가하실 것을 제안드립니다. 이렇게 조금 더 자세한 PR 설명을 전달해주시는 습관을 지금부터 길러보시면 현업에서도 도움이 되실 거예요. 😄
## 요구사항
### 해당 미션 피그마 링크
### 기본
- []
### 심화
- []
## 주요 변경사항
## 테스트 방법
### 배포 주소
### 로컬 테스트 방법
// 프로젝트 설치 및 실행 명령어와 로컬 실행 시 테스트 가능한 링크 주소
## 스크린샷
// 주요 변경사항에 대한 스크린샷
## 멘토에게
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.
권희님 꼼꼼하신 성격이시군요. 👍 너무 좋습니다.
저도 개발할 때, 개발 전에 요구사항 분석 및 설계에 대한 TO-DO list 마크다운 문서를 먼저 만듭니다.
그리고 하나씩 완료할 때마다 체크박스를 체크하고 함께 커밋하곤 합니다.
꼼꼼함이 참 중요한 것 같아요.
|
||
### 주의 사항 | ||
|
||
- [x] 판다마켓 로고 선명하지 않음 -> png에서 svg로 변경 |
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.
이러한 판단도 좋네요!
index.html
Outdated
@@ -0,0 +1,123 @@ | |||
<!DOCTYPE html> | |||
<html lang="en"> |
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.
HTMLElement.lang 속성은 요소의 특성 값과 텍스트 내용의 기본 언어를 가져오거나 설정해요.
한국어 사용자 대상으로 하는 사이트이므로 "en"이 아닌 "ko"가 되어야 해요. 😄
참고: https://developer.mozilla.org/ko/docs/Web/API/HTMLElement/lang
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.
피드백 사항에 맞추어 변경하였습니다.
index.html
Outdated
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> |
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.
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.
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.
넵 맞습니다 권희님~! 그리고 안정적인 뷰포트 메타 태그 설정 기본은 <meta name="viewport" content="width=device-width,initial-scale=1.0>
이 맞습니다.
그런데 개발하신 화면에서 모바일 반응형 뷰포트에서 레이아웃이 깨져서, minimum-scale, maximum-scale도 1 배율로 지정하는 ㅂ아법도 제안드린거예요.
<img | ||
class="home-top__img" | ||
src="/assets/img/Img_home_top.png" | ||
alt="home-top-img" |
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 속성을 잘 적용해주셨네요. 👍 다음과 같은 이유로 img 태그에 시각 장애인분께 들릴 해당 이미지를 설명하는 문구가 무엇이면 좋을지 고민하며 적절한 값을 넣어주는 것이 좋습니다.
img 태그의 alt 속성은 이미지를 보여줄 수 없을 때 해당 이미지를 대체할 텍스트를 명시합니다.
이러한 alt 속성은 사용자가 느린 네트워크 환경이나 src 속성값의 오류, 시각 장애인용 스크린 리더의 사용 등 어떤 이유로든 사용자가 이미지를 볼 수 없을 때 이미지 대신 제공할 대체 정보를 제공합니다.
<div class="home-bottom"> | ||
<div class="home-bottom__container"> | ||
<h1 class="home-bottom__title"> |
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.
BEM 방법론을 적용하여 클래스 이름을 잘 지어주셨네요 👍 덕분에 클래스 이름만으로 마크업 구조를 알 수 있게 되어, 코드 가독성이 향상됐네요.
- 참고
BEM 설명: https://nykim.work/15
index.html
Outdated
<a href="https://www.facebook.com/" target="__black"> | ||
<img src="assets/logo/ic_facebook.svg" alt="facebook" /> | ||
</a> | ||
<a href="https://www.twitter.com/" target="_blank"> | ||
<img src="assets/logo/ic_twitter.svg" alt="twitter" /> | ||
</a> | ||
<a href="https://youtube.com/" target="__black"> | ||
<img src="assets/logo/ic_youtube.svg" alt="youtube" | ||
/></a> | ||
<a href="https://www.instagram.com/" target="__black"> | ||
<img src="assets/logo/ic_instagram.svg" alt="instagram" /> | ||
</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.
앵커 태그(= a 태그)의 target 속성에 입력한 값들에 오타가 있어요. 😢
의도하신 바는 "_blank" 였겠네요. 오타 수정 부탁드려요. 🙏
그리고 target 속성에 전달되는 값의 의미가 무엇인지도 이번 기회에 정확하게 확인해보시길 바랄게요. 링크 첨부해드려요.
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 태그에 target="_blank" 속성을 사용할 때는 rel="noopener noreferrer" 특성을 지정하여 window.opener 참조 설정을 방지하는 것이 좋습니다. 이를 통해 생성된 창의 opener 속성에 접근해도 null을 반환합니다. 하이퍼링크를 신뢰할 수 없을 때, 브라우저의 보안 리스크를 방지할 수 있습니다.
관련해서 참고하실 수 있는 링크 전달드립니다.
https://mathiasbynens.github.io/rel-noopener/#recommendations
하이러링크를 신뢰할 수 없다면? noopener, noreferrer, nofollow
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.
오타가 있었네요! 피드백 감사합니다
@@ -0,0 +1,143 @@ | |||
/* http://meyerweb.com/eric/tools/css/reset/ |
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 리셋 또는 normalize가 왜 필요한지에 대해 이해하시면 좋아요.
CSS 리셋은 일관성 없는 다양한 브라우저의 기본 스타일을 초기화하는 목적으로 사용한답니다.
크롬, 사파리, 파이어폭스, 삼성인터넷 등 많은 브라우저들이 각각 HTML 태그에 대한 스타일 기본값이 다르므로 그 차이를 없애기 위해 필요해요.
즉, 크로스 브라우징을 위해 필요합니다.
웹 개발에서 "크로스 브라우징"도 매우 중요한 부분입니다.
그리고 참고로 CSS Reset에서 각 설정이 이유가 무엇인지 아래 링크 통해서 확인해보세요.
styles/main.css
Outdated
.header__login-btn { | ||
cursor: pointer; | ||
background-color: var(--main-color); | ||
padding: 12px 20px; | ||
border-radius: 8px; | ||
color: white; | ||
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
width: 128px; | ||
height: 48px; | ||
font-size: 16px; | ||
font-weight: 600; | ||
} |
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.
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.
피드백 사항에 맞추어 변경하였습니다.
추가 변경 사항
z-index 값을 관리하기 위해 변수로 지정하였습니다.
버튼 외에도 각 section의 설명 부분이 너비가 변경되지 않도록 flex-shrink: 0;
으로 주었습니다.
추가 질문
width 대신 min-width와 max-width 속성을 언제 사용하면 좋을까요?
반응형으로 프로젝트를 만들 때는 모든 width를 max-width나 min-width로 변경해줘야 하는지도 궁금합니다.
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.
모든 width를 max-width나 min-width로 변경할 필요는 없어요.
필요한 경우에 적절하게 고치면 됩니다.
각각 언제 사용하면 좋을지는 MDN 정의를 이해하신 후 적용하시면 될 것 같아요. 😄
https://developer.mozilla.org/ko/docs/Web/CSS/max-width
https://developer.mozilla.org/ko/docs/Web/CSS/min-width
display: flex; | ||
justify-content: center; | ||
align-items: center; | ||
background-color: var(--main-color); |
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.figma.com/file/IVkRlYWHY74QlgmxqA99Ym/%EC%8A%A4%ED%94%84%EB%A6%B0%ED%8A%B8-%EB%AF%B8%EC%85%98?type=design&node-id=55-1511&mode=design&t=01mJHNK3dLXcQbpC-0
기본
심화
주요 변경사항
테스트 방법
배포 주소
https://panda-market-alexgoni.netlify.app/
로컬 테스트 방법
스크린샷
멘토에게