Skip to content

구현 과제 - 로그인/회원가입 모달 - 효은#32

Open
chohe3212 wants to merge 2 commits intomainfrom
11-hyoEun
Open

구현 과제 - 로그인/회원가입 모달 - 효은#32
chohe3212 wants to merge 2 commits intomainfrom
11-hyoEun

Conversation

@chohe3212
Copy link
Copy Markdown

@chohe3212 chohe3212 commented Aug 6, 2023

로그인 페이지 구현 완료하였습니다.

#To do.

  • 체크박스 구현 완

@chohe3212 chohe3212 changed the title 구현 과제 - 로그인 모달 및 기능 구현 - 효은 구현 과제 - 로그인/회원가입 기능구현 - 효은 Aug 6, 2023
@chohe3212 chohe3212 changed the title 구현 과제 - 로그인/회원가입 기능구현 - 효은 구현 과제 - 로그인/회원가입 모달 - 효은 Aug 6, 2023
@chohe3212 chohe3212 linked an issue Aug 6, 2023 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@YennieJ YennieJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

따로 설정한게 없다면,
font-size: 16px;
font-weight: 400;
font-style: normal;
line-height: normal;
이 기본이라서 모든 태그에 추가 할 필요는 없습니다!

p태그를 주로 사용하셨는데 span태그 사용을 고려해보시는것도 좋을꺼같아요

그리고 가독성을 위해서 주석처리 + CSS의 경우 같은 맥락에 있는 친구들은 엔터 없이 묶어 놓는 것도 좋은 방법인거 같습니다 :)

고생하셨습니다 효은님 🙆🏻‍♀️

css/style.css Outdated
Comment on lines +29 to +32
display: inline-block;
width: 520px;

margin:0%;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

display: inline-block; margin:0%;
을 쓰신 이유가 있을까여?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아뇨.. 코드 지웠습니다!!

Comment on lines +17 to +26
body{
width: 100%;
height: 100vh;
margin:0;
display: flex;
justify-content: center;
align-items: center;
background-color: #F2F2F2;

}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

화면이 위아래로 작아졌을 때 컨텐츠가 짤려요 :(
justify-content: center;
align-items: center;
으로 중앙 정렬을 하기 보단 main에서 margin으로 조절 하는것도 좋을꺼 같습니다:)

  • body 자체에 font-family 설정하기

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오오 맞네요!! min-height: 100vh; 로 해결했습니다 감사해요 :)

index.html Outdated
Comment on lines +19 to +27
<input type="text" name="id" id="id" placeholder="아이디" />
<p class="warning">아이디를 입력해 주세요</p>
<input type="password" name="pw" id="pw" placeholder="비밀번호" />
<p class="warning">아이디 혹은 비밀번호과 일치하지 않습니다.</p>
<div class="keep-login">
<input type="checkbox" id="inphold" class="txt-hide" />
<label for="inphold" class="labl-hold">로그인 상태 유지</label>
</div>
<button class="login-button">로그인</button>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

form 태그를 활용해서 입력된 사용자 정보가 서버에서 확인 될 수 있게 만드는게 좋을꺼 같아요 :)
form에 padding 값을 넣으면 내부의 모든 요소에 margin-left를 사용하지 않아도 됩니다:)!
button 에는 type="submit"도 추가하구요!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵!!! 좋은 충고 감사합니다 : )

Comment on lines +184 to +188
.login .signin-container .signin::after{
margin: 0 12px 0 12px;
content : "|";

}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<a href="#" class="signin"> 자체에 after를 주게되면 after도 a태그 영역으로 묶이게 되기 때문에 "|" 가상요소 또한 a태그가 됩니다.
이런 경우는 ul>li>a로 마크업을 구성하고 li에 가상요소를 주면 해당 문제는 해결된다고 저희 멘토님께 답변을 받았습니다 :)
참고하시면 좋을꺼같아요

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

맞습니다. 현재 | 부분에도 cursor가 pointer로 작동하고 있습니다.
위에 예진님처럼 가상요소를 주시거나 |이 들어간 p태그를 하나 추가해주시고 a 태그에만 cursor를 pointer로 주고
signin-container 클래스에 gap을 줘서 간격을 벌리는 방법도 있습니다.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵!! 말씀해주신대로 수정 했습니다 감사합니다

@merrybmc
Copy link
Copy Markdown
Collaborator

merrybmc commented Aug 7, 2023

효은님 첫 pr 게시에 성공한걸 축하드려요 🎉🎉🎉

구현한걸 저도 봤는데 전체적으로 깔끔하게 구현도 엄청 잘해주시고
피드백을 이미 예진님이 제가 하고 싶은 말을 다 남겨주셔서 따로 없네요. 😭

실제 sns 로그인은 아니지만 button 그룹에 클릭하면 해당 링크로 이동하는 디테일도 좋았어요. 👍

다음 JS때 또 봐요 😀

@chohe3212
Copy link
Copy Markdown
Author

두분 너무 감사드립니다ㅠㅠ 코드 수정하면서 배우는 점이 훨씬 많다는 것을 느꼈어요!! 앞으로 저도 코드리뷰 활발히 달아보겠습니다! 좋은 코드리뷰 최고에요..👏👍😄

@AYFG
Copy link
Copy Markdown

AYFG commented Aug 7, 2023

안녕하세요 효은님!! 꼼꼼하게 잘구현하셨네요 고생많으셨습니다!👍👍

body {
  /* justify-content: center; */
}

main {
  margin: auto;
}

개인적인 의견 조금 남겨봅니다..! 확대를 최대로 하였을 때 justify-content: center;가 있어서 컨텐츠를 제대로 볼 수 없는 현상이 있더라고요! body에 justyfi-content:center를 주는 것 대신 main에게 margin:auto를 주어 가운데로 정렬하면 최대로 확대해도 컨텐츠를 이용할 수 있어 웹 접근성에 더 좋을 것 같다고 생각하여 남깁니다😊 앞으로 잘부탁드립니다!!

@chohe3212 chohe3212 self-assigned this Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

구현 과제 - 로그인/회원가입 모달

4 participants