-
Notifications
You must be signed in to change notification settings - Fork 39
[유동환] sprint2 #95
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
[유동환] sprint2 #95
The head ref may contain hidden characters: "Basic-\uC720\uB3D9\uD658-sprint2"
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.
동환님 2번째 스프린트 미션 제출 고생하셨습니다~
맵게 리뷰해달라고 하셔서 조금 어려운 부분들도 코멘트 드렸으니
너무 어려우시면 쉬운 것부터 적용해보세요~
다음번 미션 제출도 화이팅입니다!
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.
👍 칭찬
깃에 올라가지 않을 파일을 관리해주시는 것 좋습니다~
image/판다마켓.png
Outdated
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.
❗️ 수정요청
이미지 파일명은 영어로 작성하는 것이 좋습니다.
한글 파일명을 사용하면 브라우저 호환성 문제나 URL 인코딩 이슈가 발생할 수 있고 기본적으로 영어를 사용하는 것이 룰이니 이를 지키는 것이 좋습니다.
따라서 판다얼굴.png는 logo.png로 판다마켓.png는 typography.png와 같은 파일명으로 변경하시는 것을 추천드릴께요~
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
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"> | ||
| <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> |
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.
💊 제안
타이틀은 브라우저 탭에 표시되는 정보로 주로 어떤 사이트의 어떤 페이지에 머무르고 있는지를 표시합니다.
판다마켓 - 로그인 과 같은 형식으로 기입하셔서 유저에게 자세한 정보를 제공하시는 것을 추천드려요!
| <div class="login-button"> | ||
| 로그인 | ||
| </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.
❗️ 수정요청
해당 태그는 로그인 동작을 수행하는 태그이므로 button 태그가 더 적절합니다.
즉 form에서 제출 이벤트가 발생했을때 해당 form 안의 유저가 입력한 정보를 제출하는 역할을 하므로
button으로 작성해주세요!
| <div class="login-button"> | |
| 로그인 | |
| </div> | |
| <button type="submit" class="login-button"> | |
| 로그인 | |
| </button> |
button 태그에서 type 속성은 해당 태그의 동작과 연관이 되어 있어 가능하면 늘 기입하시는 것이 좋습니다.
로그인 페이지의 form에서 로그인 버튼, 회원가입 페이지의 form에서 회원가입 버튼과 같은 경우는 submit 타입이고 그외 대부분의 버튼은 button 타입입니다.
| <img src="./image/판다마켓.png"> | ||
| </a> | ||
| </div> | ||
| <form> |
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.
👍 칭찬
form 태그로 관련된 요소만 묶어 주신 것 좋습니다~
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 파일을 작성하시면 유지보수 및 가독성 측면에서 더 좋을 것 같습니다.
auth.css // login, sign-up 페이지에서 공통으로 쓰는 것들의 모음
login.css
signup.css
만약 login.css, signup.css 파일에서 작성되는 css가 너무 적다면, 즉 분리할 필요를 못 느끼신다면
하나로 합치셔서 관리해도 좋을 것 같아요~
| text-decoration: none; | ||
| } |
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.
❗️ 수정요청
.footer-mid > a 태그에 기본적으로 적용되어 있는 text-decoration 속성을 none으로 변경해주신 점 좋습니다!
다만 a 태그는 text-decoration 속성 외에도 color 값이 blue로 지정되어 있으니 이도 디자인이랑 동일하게 변경해주세요~
| align-items: center; | ||
| } | ||
|
|
||
| container { |
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.
❗️ 수정요청
container라는 태그는 없으니 오타이신 것 같아요~
| container { | |
| .container { |
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.
이부분은 body 다음에 가장 크게 container 로 한 번 감싸주었습니다. 아마도 나중에 큰 틀을 정리하기 쉽게 하기 위한 것 같습니다. 이렇게 큰 태그로 한 번 감싸주는것이 괜찮을까요?
| } | ||
|
|
||
| /* 폼테그 관련 */ | ||
| form { |
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.
💊 제안
태그 선택자는 해당 태그가 모든 페이지에 걸쳐 적용되어야 하는 스타일링 외에는 사용을 추천드리지 않습니다~
위의 경우도 프로젝트가 확장되다보면 변경될 수 있으니 가능한 class를 사용하시는 것을 추천드릴께요~
예를 들어, 1월에는 auth.css를 사용하는 페이지가 로그인, 회원가입만 존재하고 해당 페이지의 form 요소가 gap이 2.4rem을 가지고 있었지만 2월에 비밀번호 찾기라는 auth 관련 새 페이지가 추가될 수 있습니다. 해당 페이지는 다른 페이지들처럼 디자인은 거의 비슷하지만 사용되는 form 요소는 gap이 1rem일 수 있습니다. 따라서 이렇게 디자인에 따라 다양하게 적용될 수 있는 스타일의 경우 classname을 통해 개별로 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.
여기 답변이 있었군요 ㅎㅎ
되도록이면 class를 사용하도록 하겠습니다
근데 위에처럼 한 번 더 감싸는 부분은 해도 좋은지 궁금합니다.
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게