-
Notifications
You must be signed in to change notification settings - Fork 39
[유동환] sprint3 #148
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 #148
The head ref may contain hidden characters: "Basic-\uC720\uB3D9\uD658-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번째 미션 제출 고생하셨습니다.
디자인대로 잘 구현하셔서 피드백 드릴 것이 크게 없었습니다~
다음 미션도 화이팅입니다!
-
아직 css 정리 못하였습니다. ( div 남발.):
코멘트도 남겼지만 css에서 중복되는 내용이 보입니다. 추후 중복을 줄이시면 좋겠습니다! -
심화과정 아직입니다.:
기본 요구사항을 잘 구현하셔서 심화도 잘 하실 것 같아요~ -
더 좋은 css 와 html을 만들려면 어떻게 구조를 짜면 좋을까요?:
좋은 css에 대한 정의에 따라 다르겠지만 저는 중복이 적고 요구사항을 만족시키는 것이 좋은 css라고 생각합니다. 좋은 HTML이란 구조와 영역의 구분이 명확하고, 상황에 따라서는 시멘틱한 것을 의미할 것 같아요. 이러한 기준을 스스로 세워보시고 이에 따라 구조를 짜보시면 좋을 것 같습니다.
| box-sizing: border-box; | ||
| } | ||
|
|
||
| @media (min-width: 375px) { |
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를 조건문에서 빼주시는 것을 추천드려요~
| <link rel="stylesheet" as="style" crossorigin href="https://cdn.jsdelivr.net/gh/orioncactus/[email protected]/dist/web/variable/pretendardvariable.min.css" /> | ||
| <link rel="stylesheet" href="login.css"> | ||
| <title>Document</title> | ||
| <link rel="stylesheet" href="login-Signup.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.
💊 제안
지금 코드를 봤을 때는 login-Signup.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.
💊 제안
이미 -로 단어를 구분해주셨으니 signup이 대문자로 시작할 필요가 없을 것 같아요. login-signup으로 변경하시는 것을 추천드려요!
| @media (min-width: 768px) { | ||
| /* 버튼 관련 */ | ||
| .buttons { | ||
| width: 100%; | ||
| height: 56px; | ||
| border-radius: 40px; | ||
| background-color: #9CA3AF; | ||
| display: flex; | ||
| flex-direction: row; | ||
| justify-content: center; | ||
| align-items: center; | ||
| color: #F3F4F6; | ||
| margin-bottom: 24px; | ||
| font-size: 20px; | ||
| font-weight: 600; | ||
| border: 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.
❗️ 수정요청
해당 미디어 쿼리 조건을 보면 @media (min-width: 375px) 로 375px 이상의 화면에서는 해당 css가 적용되게 됩니다. 지금 작성하신 미디어 쿼리 조건인 @media (min-width: 768px)를 만족하면서 그 전 조건도 만족할 수 있기 때문에 중복으로 스타일링을 해주실 필요가 없습니다.
변경이 필요한 경우만 작성해주시고 중복되는 것은 지워주시는 것이 성능, 유지보수 및 가독성 측면에서 유리합니다~
| @media (min-width: 768px) { | |
| /* 버튼 관련 */ | |
| .buttons { | |
| width: 100%; | |
| height: 56px; | |
| border-radius: 40px; | |
| background-color: #9CA3AF; | |
| display: flex; | |
| flex-direction: row; | |
| justify-content: center; | |
| align-items: center; | |
| color: #F3F4F6; | |
| margin-bottom: 24px; | |
| font-size: 20px; | |
| font-weight: 600; | |
| border: 0px; | |
| } | |
| @media (min-width: 768px) { |
| form > input { | ||
| width: 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.

요구사항
기본
랜딩 페이지
로그인, 회원가입 페이지 공통
심화
주요 변경사항
스크린샷
멘토에게