-
Notifications
You must be signed in to change notification settings - Fork 39
[김태일] Sprint3 #158
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 #158
The head ref may contain hidden characters: "Basic-\uAE40\uD0DC\uC77C-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 올려주실 때는 배포해서 주소도 같이 올려주시면 좋을 것 같습니다!
| width:40rem; | ||
| margin: 14.4375rem auto; | ||
| height: 38.625rem; | ||
| /* gap:2.5rem; */ |
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.
💊 제안
테스트하시다가 남겨두신 코드라면 주석으로 두시는 것보다 지우시는 것이 가독성과 유지보수에 더 좋습니다~
| margin-top:40px; | ||
|
|
||
| .title img { | ||
| width: 6.47rem; |
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.
❗️ 수정요청
rem이라는 상대 단위를 사용하신 점은 좋습니다!
다만, rem은 루트 요소(html)의 font-size를 기준으로 동작하기 때문에, 보다 일관된 계산을 위해 html 태그에 font-size: 10px을 설정하는 것을 추천드립니다.
현재 코드를 예로 들면, width: 6.47rem은 기본적으로 브라우저의 기본 폰트 크기(16px)를 기준으로 계산되어 16 × 6.47 = 103.52px이 됩니다.
하지만 html에 font-size: 10px을 설정하면, 1rem = 10px이 되어 계산이 더욱 직관적이고 유지보수하기 쉬워집니다.
지금 코드에서처럼 img의 width를 70px로 설정하고 싶다면, width: 10.3rem을 입력하면 됩니다.
| .group label { | ||
| width: 2.9375rem; | ||
| height: 1.625rem; | ||
| font-family: Pretendard; |
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.
💊 제안
이렇게 모든 태그에 font-family 값을 Pretendard로 주시는 것보다 최상위 태그에 해당 속성을 명시하고 자식 태그들은 상속받게 하는 것이 중복을 줄일 수 있고 가독성 측면에서도 더 좋을 것 같습니다~
| gap: 0.625rem; | ||
| border-radius: 0.75rem; | ||
| background-color: #F3F4F6; | ||
| border: 2px; |
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.
💊 제안
border라는 css 속성은 단축 속성으로 실제로는 border-width, border-style, border-color 속성을 하나의 속성으로 적는 것입니다. 지금의 경우 border-width만 작성되어 있으니 이런경우 border-width로 적어주시는 것이 더 명확하고 가독성 측면에서도 좋습니다. 만약 다른 속성을 적지 않으신 거라면 적어주시는 것을 추천드려요!
| border: 2px; | |
| border-width: 2px; // border-width만 필요할 경우 이렇게 | |
| border: 2px solid var(--blue); // border를 적을경우 모든 속성을 기입 |
| /* 모바일 (375px ~ 767px) */ | ||
| @media screen and (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.
💊 제안
한 레포안에서는 동일한 룰을 따르는 것이 좋습니다.
따라서 signin, signup 처럼 쓰시거나 sign-in, sign-up처럼 같은 네이밍 룰을 따르시는 것을 추천드려요~
| .contents0 img { | ||
| width: 46.625rem; | ||
| height: 21.25rem; |
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.
❗️ 수정요청
이렇게 height, width 속성을 동시에 주게 되시면 이미지의 원본 비율을 고려해 작성해주신 것이 아니라면 지금처럼 비율이 달라지게 됩니다!
구현할때 이미지의 비율을 유지하는 것이 중요하므로 비율을 유지할 수 있는 방향으로 수정해보세요!
| .contents0 img { | |
| width: 46.625rem; | |
| height: 21.25rem; | |
| .contents0 img { | |
| width: 46.625rem; | |
| height: auto; |
요구사항
기본
와 “로그인" 버튼의 간격이 가까워집니다.
와 “로그인" 버튼의 간격이 가까워집니다.
심화
이지 메타 태그를 설정해 주세요.
주요 변경사항
스크린샷
멘토에게