-
Notifications
You must be signed in to change notification settings - Fork 39
[김정우] Sprint2 #98
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 #98
The head ref may contain hidden characters: "Basic-\uAE40\uC815\uC6B0-sprint1"
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주차 미션 제출 고생하셨습니다!
이번에 구현하신 로그인, 회원가입 페이지에서 눈 모양 아이콘이 빠져있으니
다음에 같이 구현해서 제출하시면 좋겠습니다~
3주차도 화이팅입니다!
- 다음에는 PR 올리실때 PR 템플릿에 맞춰 요구사항 기입하시고 완료한 것들 체크해주세요~ 해당 내용을 기입해주시면 코드 리뷰에 도움이 됩니다~
- netlify 주소 첨부해주신 것 좋습니다.
비밀번호 확인을 어떻게 보이게 할 수 있는지 모르겠습니다. name하고 id를 비밀번호하고 똑같이 했는데, 혹시 문제가 될지도 궁금합니다.:
관련해서 코멘트 드렸으니 확인해보세요~
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.
❗️ 수정요청
해당 파일은 mac에서 작업하실 때 생성되는 파일입니다.
해당 폴더에 대한 정보를 가지고 있는 파일로, 이러한 파일들은 git repo에 올라갈 이유가 없습니다~
git repo에는 공유할필요가 있는 파일들만 올라가면 되기 때문에 이러한 파일들은 git에 올라가지 않도록 관리하는 것이 좋습니다.
관련한 블로그 글이 있으니 읽어보시고, 제거하시는 것을 추천드릴께요~
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라는 확장자를 통해 style 파일이라는 것을 알 수 있으므로 이름에서 style을 제외하시면 더 좋을 것 같아요~
또한 해당 파일 하나를 통해 login.html과 signup.html에서 같이 사용하고 있으시니 auth.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.
👍 칭찬
중복된 코드를 줄이고자 하나의 파일로 작성하신 점 좋습니다 👍
| * { | ||
| box-sizing: border-box; | ||
| } | ||
|
|
||
| html { | ||
| font-family: Pretendard, sans-serif; | ||
| word-break: keep-all; | ||
| } |
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 파일에 적어주시기보다
reset.css를 분리하신 것처럼 global.css 파일을 생성하셔서 해당 파일 에 적어주시는게 global 이라는 파일명에도 더 적절하고 관리하기도 편하실 것 같습니다~
| color: #3692FF; | ||
| } | ||
|
|
||
| #nicname { |
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.
💊 제안
아이디는 한 페이지에서 고유해야 하는 값입니다. 이러한 특성으로 classname과 달리 재활용이 어렵고 우선순위가 높기 때문에 덮어씌우기가 어렵습니다. css 작성시에는 클래스 선택자 사용을 추천드립니다~
아이디 선택자는 css 작성시보다 JS, 혹은 특정 태그를 유일하게 식별해야할 때 사용하시는 것을 추천드려요!
| <div class="content-login"> | ||
| <a class="login-button" href="/signup">회원가입</a> | ||
| </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="content-login"> | |
| <a class="login-button" href="/signup">회원가입</a> | |
| </div> | |
| <div class="content-login"> | |
| <button type="submit" class="login-button">회원가입</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 영역은 회원가입할때 필요한 데이터들의 구역을 의미합니다.
간편 로그인하기 부분은 회원가입시 필요한 정보가 아니니 form 태그 밖에 두시면 됩니다~
이를 참고해서 form 태그를 추가해보세요!
| </div> | ||
| </div> | ||
| <div class="foot-content"> | ||
| <p class="insa">판다마켓은 처음이신가요?</p> |
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.
❗️ 수정요청
변수나 클래스명, 이미지명과 같은 이름은 해당 파일이 어떤 역할을 하는 친구인지 나타내는 큰 단서가 되고 코드파악 및 유지보수에 중요한 역할을 합니다.
insa 라는 클래스명은 해당 태그를 잘 설명하지 못하니 변경해주시는 것을 추천드려요~
(insta라는 의미인지 인사라는 의미인지 의도가 궁금하네요~)
| <html lang="ko"> | ||
| <head> | ||
| <meta charset="utf-8"> | ||
| <title>회원가입</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.
💊 제안
타이틀은 브라우저 탭에 표시되는 정보로 주로 어떤 사이트의 어떤 페이지에 머무르고 있는지를 표시합니다.
{사이트명} - {페이지명} 과 같은 형식으로 기입하셔서 유저에게 자세한 정보를 제공하시는 것을 추천드려요!
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.
💊 제안
.github에 있던 파일을 밖으로 빼신 것 같아요~
다만 1주차에서 수정이 되지 않은 것 같으니 1주차 리뷰를 확인하시고 해당 파일의 코멘트들을 반영하시면 좋을 것 같습니다~
| <p>비밀번호</p> | ||
| <input type="password" name="userPassword" id="userPassword"> | ||
| <p>비밀번호 확인</p> | ||
| <input type="password" name="userPassword" id="userPassword"> |
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.
❗️ 수정요청
input의 name 속성은 해당 input값을 구분하는데 사용되는 속성으로 input의 경우 중복되면 안됩니다~
id 속성도 페이지당 유일해야하는 값이므로 두 input의 name, id값을 다르게 해주세요!
| <p>비밀번호</p> | |
| <input type="password" name="userPassword" id="userPassword"> | |
| <p>비밀번호 확인</p> | |
| <input type="password" name="userPassword" id="userPassword"> | |
| <p>비밀번호</p> | |
| <input type="password" name="userPassword" id="userPassword"> | |
| <p>비밀번호 확인</p> | |
| <input type="password" name="userPasswordCheck" id="userPasswordCheck"> |
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게
-https://pandafe15.netlify.app/ = netlify주소입니다.