-
Notifications
You must be signed in to change notification settings - Fork 39
[김동한] Sprint3 #105
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
[김동한] Sprint3 #105
The head ref may contain hidden characters: "Basic-\uAE40\uB3D9\uD55C-sprint3"
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.
동한님 3번째 PR 제출 수고하셨습니다!
반응형 작업을 잘하셔서 사이트가 잘 동작하네요.
다음 미션 제출도 화이팅입니다!
요구 사항이 잘 반영되었는지 궁금합니다.: 넵 잘 구현하셨습니다.og 메타 태그가 맞게 사용된 것인지 궁금합니다.: 코멘트 참고해주세요~전체적으로 개선해야할 코드들이 있다면 어떤 것들이 있는지 궁금합니다.: 좋은 방향으로 작업하고 계시다고 생각합니다. 다만 가독성에 대해 고민해보시면 더 실력이 향상되실 것 같습니다. 특정 css가 이해가 잘 가는지, 특정 클래스명을 통해 태그에 대해 추측이 가능한지 등을 고민해보세요!
| width: 100%; | ||
| height: 100%; |
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를 작성하셔도 상관이 없지만
일반적으로 width, height 속성을 100%로 설정하면 기존 이미지의 비율이 깨질 수도 잇어 추천드리지 않습니다~
이러한 경우 width: 100% 만 있어도 height는 기존 이미지의 비율인 1:1을 유지하기 위해 자동으로 커집니다~
| width: 100%; | |
| height: 100%; | |
| width: 100%; |
|
|
||
| .visible_on, | ||
| .visible_off { | ||
| .visible_button { |
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 (max-width: 744px) { | ||
| #user_form_logo { | ||
| @media (min-width: 375px) and (max-width: 767px) { |
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.
💊 제안
min-width가 조건에 걸려있어 375px 미만으로 화면이 작아지게 되면 PC 스타일이 적용되네요~
body에 min-width: 375px를 추가하셔서 해당 사이즈 미만으로 작아지게 되면 가로 스크롤이 생기게 해주시거나
아니라면 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.
💊 제안
특정 코드를 분류하는 기준을 추측할 수 있는 가장 큰 단서가 이름이라고 생각합니다.
reset 이라는 파일명을 통해 브라우저 스타일을 초기화하는 코드의 모음이라고 예상할 수 있습니다.
common이라는 파일명을 통해서는 프로젝트에 걸쳐 사용되는 코드라고 예상이 가능합니다.
지금의 경우 common안에 reset 코드가 같이 존재하고 있어 이러한 기준과 잘 맞지 않는다고 생각합니다!
저는 개인적으로 기존의 reset.css를 살리시고 나머지를 common.css로 분리하시는 것을 추천드려요!
| <meta property="og:title" content="판다마켓" /> | ||
| <meta property="og:type" content="website" /> | ||
| <meta property="og:url" content="https://pandaamarket.netlify.app/" /> | ||
| <meta property="og:image" content="https://pandaamarket.netlify.app/image/logo.png" /> | ||
| <meta property="og:image:type" content="image/png" /> | ||
| <meta property="og:image:width" content="792" /> | ||
| <meta property="og:image:height" content="264" /> | ||
| <meta property="og:description" content="일상의 모든 물건을 거래해보세요." /> |
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 src="/image/logo.png" alt="판다마켓 로고" class="logo" /> | ||
| <img src="./image/mobile_logo.png" alt="판다마켓 모바일 로고" class="m_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.
💊 제안
이렇게 뷰포트별로 이미지를 렌더링하실거라면 picture 태그 사용을 추천드려요~
지금처럼 css를 통해 디자인을 구현하실거라면, 로고에서 얼굴 부분과 "판다마켓"이라는 글씨를 분리하셔서 css로 작업하시는 것을 추천드립니다~
| <h3 class="category_title">Register</h3> | ||
| <h2 class="title"> | ||
| 판매를 원하는<br /> | ||
| <span>판매를 원하는 </span> |
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.
❗️ 수정요청
지금 디자인에서 괜찮을 것 같지만 공백은 의도치 않은 동작을 발생시킬 수 있습니다~
필요한 공백이 아니면 가능하면 습관적처럼 없애는 것을 추천드려요~
| <span>판매를 원하는 </span> | |
| <span>판매를 원하는</span> |
| <input type="password" id="password" name="password" /> | ||
| <img src="./image/btn_visibility_on.png" alt="비밀번호 보기" class="visible_on" /> | ||
| <input type="password" id="password" name="password" placeholder="비밀번호를 입력해주세요" /> | ||
| <button class="visible_button"> |
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.
❗️ 수정요청
버튼 태그는 type 속성을 명시해주지 않으면 submit 을 기본으로 가지게됩니다.
submit 타입의 버튼은 form 태그를 제출하는 역할을 하기때문에 해당 태그에는 button 타입이 적절합니다~
| <button class="visible_button"> | |
| <button type="button" class="visible_button"> |
type 속성은 버튼 동작에 있어 중요한 속성이므로 습관적으로 기입해주시는 것을 추천드려요~

요구사항
기본
심화
주요 변경사항
스크린샷
![URL]https://pandaamarket.netlify.app/



멘토에게