-
Notifications
You must be signed in to change notification settings - Fork 31
[박원현] sprint2 #58
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 #58
The head ref may contain hidden characters: "Basic-\uBC15\uC6D0\uD604-sprint2"
Conversation
… 클래스 명을 수정함/ 폼 내부의 Input값이 모두 채워지면 버튼의 색이 변하는 css 추가함
dongqui
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.
원현님 이번 미션도 훌륭하게 잘 마무리 하셨네요!! 👍
코드를 구조화 시키시려는 모습이 보기 좋습니다. html 시맨틱에 조금만 더 신경써주시면 더욱 좋을 거 같아요.
다양하게 시도해보고 받아들이시는 것을 보고 성장이 굉장히 빠르다고 느꼈어요! 앞으로가 기대 됩니다 :)
처음에 git pull을 안하고 로컬에서 Basic-박원현-sprint2 브랜치를 생성해서 push를 하는 바람에 해결한다고 3시간 정도 걸린 것 같습니다. 로컬에서 만든 브랜치를 PR하려고 하니 commit의 history가 일치하지 않는다는
image
이런 오류 메시시가 발생한 것이었습니다. 강사님께서 제 커밋이력을 보시고 제가 해결한 방법이 적절한 방법이었는지 확인해 주실 수 있으실까요?(저는 branch를 기존 Basic-박원현에서 새롭게 생성해 기존에 로컬로 생성한 branch와 merge해서 해결하고자 했습니다.)
-> 커밋 내용만 봤을 때는 어떤 일이 발생한 건지 파악하기가 어렵네요 😢

다만 문제가 된 브랜치가 어떤 커밋에서 부터 시작된 것인지 확인이 안 됩니다..! git pull을 안 했거나, 로컬에서 한 것과는 상관 없이 해당 브랜치는
main -> Basic -> Basic-박원현 -> Basic-박원현-sprint-1
로 이어져있는 커밋 줄기 어딘가에서 뻗어나왔어야 합니다! 깃은 단순히 파일이 변경되었는 지를 따지는 것이 아니라 이전 커밋을 기반으로 변경 사항을 확인합니다.
머지가 되기 위해서는 같은 줄기의 커밋 기록을 가져야 해요.
일단 머지는 잘 된 거 같아서 해결은 된 거 같은데.. 적절한 해결 방법인지 판단 전에, 해당 경우는 발생하면 안 됩니다 😢 같은 줄기의 커밋에서 시작했다면 rebase등을 통해 비교적 쉽게 해결이 가능했을 거에요!
로컬에서 작업하면서 제 나름대로 분리하고 의미대로 commit 메시지를 작성했습니다. 적절한 메시지라고 생각되시는지/ 부족한 부분이 있다면 어디를 신경써서 작성하면 좋을지 말씀해주시면 감사하겠습니다.
-> 작업 단위로 커밋을 잘 나눠주셨고, 해당 작업을 잘 표현해주신 점에서 좋다고 생각합니다. 해당 부분은 보통 회사나 팀 차원에서 컨벤션을 맞추는 경우가 많아요~! 구글을 참고 링크로 남겨드립니다. https://developers.google.com/blockly/guides/contribute/get-started/commits
login 페이지와 signup 페이지를 작업하면서 그 페이지 모두 같은 ui의 form을 사용하는 것을 보고 같은 class를 사용했습니다. 결국 하나의 css 파일로 2가지 페이지를 스타일링하게 되었는데 이러한 중복되는 코드를 제거하는 파일 관리법이 적절한 관리인가요?
-> 네 좋습니다! 현재 상황에 맞게 잘 대처하셨네요 :)
form.css파일의 클래스명이 적절하다고 생각되시나요?
->BEM을 사용하셨군요! 요소가 잘 표현된 거 같습니다 👍
images/auth 에 새롭게 추가한 이미지 파일을 저장하였습니다. 이름을 적절하게 변경했는지, login과 signup을 auth라고 퉁쳐서 한 폴더에 이미지를 담았습니다.
-> 정답은 없습니다! 언제나 트레이드 오프를 잘 따져보면 좋습니다 :) 지금처럼 도메인별로 폴더를 나눈다면 나중에 페이지에 필요한 이미지를 쉽게 찾을 수 있지만, 만약 페이지가 더 추가 되고 중복되는 이미지들이 생긴다면 오히려 관리하기 어려워지겠죠!
css 스타일링 순서를 요소의 크기 -> 요소의 색상 -> 요소의 패딩이나 마진값 -> 요소의 배치,디스플레이 -> 요소 내부 텍스트 설정 로 통일성을 가져가려 신경썻습니다.
-> 코드를 구조화 하시려는 노력 너무 좋네요 :)
| /> | ||
| <div class="feature-content"> | ||
| <h2 class="feature-tag">Hot item</h2> | ||
| <h1>인기 상품을<br />확인해 보세요</h1> |
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.
h tag 는 계층 구조로 사용하시는 것이 접근성에 좋습니다! h1은 페이지에 하나가 되어야합니다 :)
https://developer.mozilla.org/ko/docs/Web/HTML/Element/Heading_Elements#사용_일람
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올릴땐 전부 검토하겠습니다.
| <a | ||
| href="https://www.facebook.com/" | ||
| target="_blank" | ||
| rel="noopener noreferrer" |
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.
opener, referrer까지 챙겨주셨군요! 👍
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.
이부분은 템플릿 코드부분이라 제가 챙긴게 아니라서 부끄럽습니다.
해당내용에 관해 학습하고 반영하도록 하겠습니다.
| /></a> | ||
| </h1> | ||
| <div class="form__group"> | ||
| <label for="email">이메일</label> |
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.
label과 input을 잘 적용해 주셨네요! 👍
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> | ||
| <button class="form__button">회원가입</button> | ||
| <div class="form__social-login"> | ||
| <p>간편 로그인하기</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.
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.
넵 확인했습니다.
span 이 적절한 선택지이겠죠?
| .form__password-toggle { | ||
| width: 24px; | ||
| position: absolute; | ||
| top: calc((100% - 24px) / 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.
absoulte를 활용해서 가운데에 위치시킬 때
top: 50%;
transform: translateY(-50%);
이런 식으로도 많이 씁니다 :)
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.
넵 좀 더 간결하게 사용할 수 있겠네요 감사합니다!
| @@ -0,0 +1,102 @@ | |||
| * { | |||
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.
이부분은 템플릿 코드부분이라 제가 챙긴게 아니라서 부끄럽습니다.
해당내용에 관해 학습하고 반영하도록 하겠습니다.
| font-family: "Pretendard", sans-serif; | ||
| } | ||
|
|
||
| header { |
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.
global css에서는 home page에서만 사용되는 스타일은 제외해주시는 게 좋겠네요 :)
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.
넵 알겠습니다!
| letter-spacing: 0.02em; | ||
| } | ||
|
|
||
| .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.
버튼 스타일을 잘 정의하셨네요! 👍
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값이 모두 채워지면 버튼의 색을 변경하는 css */ | ||
| form:has(input:invalid) .form__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.
다양한 속성을 사용하고 계시는군요! 👍👍
다만 css는 다양한 유효성 검사에 대응하기가 힘듭니다 :)
또한 버튼의 경우 단순히 스타일을 바꾸기 보다는 disabled 속성을 이용하는 경우가 많아요!
요구사항
기본
로그인 페이지, 회원가입 페이지 공통
로그인 페이지
회원가입 페이지
심화
주요 변경사항
스크린샷
멘토에게
이런 오류 메시시가 발생한 것이었습니다. 강사님께서 제 커밋이력을 보시고 제가 해결한 방법이 적절한 방법이었는지 확인해 주실 수 있으실까요?(저는 branch를 기존 Basic-박원현에서 새롭게 생성해 기존에 로컬로 생성한 branch와 merge해서 해결하고자 했습니다.)