Skip to content

구현 과제 - 로그인/회원가입 모달 - 경민#25

Open
kmryuuu wants to merge 5 commits intomainfrom
11-kyoungMin
Open

구현 과제 - 로그인/회원가입 모달 - 경민#25
kmryuuu wants to merge 5 commits intomainfrom
11-kyoungMin

Conversation

@kmryuuu
Copy link
Copy Markdown

@kmryuuu kmryuuu commented Aug 1, 2023

로그인/회원가입 첫 번째 레이아웃을 구현하였습니다.

To do

  • 닫는 아이콘 cursor: pointer; 추가
  • 폼 부분 label 태그 추가
  • 로그인 버튼 반응형 단위인 %로 수정
  • 구분선과 소셜 로그인 폰트 색상 추가
  • 소셜 로그인 a 태그에서 button 태그로 수정

Comment on lines +16 to +25
<svg
xmlns="http://www.w3.org/2000/svg"
width="18"
height="18"
viewBox="0 0 18 18"
fill="none"
>
<path d="M1.15918 16.9999L17 1.15915" stroke="#767676" />
<path d="M16.8408 16.8408L1.00003 1" stroke="#767676" />
</svg>
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.

cursor: pointer; 를 주면 더 좋을거 같아요!

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.

저도 웹접근성을 위해 X 모양 버튼을 클라이언트가 클릭할 수 있는걸 알 수 있게 cursor를 포인터로 바꿔주면 좋을 것 같습니다.

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.

오 svg에도 적용되는지 덕분에 알게 되었네요.. 두분의 의견 감사합니다!

<form>
<input type="text" class="user-id" placeholder="아이디" />
<p class="warning-id">아이디를 입력해 주세요.</p>
<input type="password" class="user-pw" placeholder="비밀번호" />
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.

아이디와 비밀번호에도 label tag를 추가하는 것도 좋을꺼같습니다!

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.

아하! 반영하겠습니다 😀


/* 로그인 버튼 */
.btn-login {
width: 472px;
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.

반응형을 위해 width를 px단위로 잡는 것은 최대한 지양한다고 합니다!
100%로 변경해도 좋을거 같아요:)

display: flex;
align-items: center;
justify-content: center;
}
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.

color를 추가해야할꺼 같습니다:)

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.

엇 color가 빠졌군요 감사합니다!

flex-direction: column;
align-items: center;
justify-content: center;
}
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.

color를 추가해야할꺼 같습니다:)

@YennieJ
Copy link
Copy Markdown
Collaborator

YennieJ commented Aug 3, 2023

경민님 주석처리로 가독성에 도움을 주셔서 감사합니다:)
전체 높이가 다른것은 경고문이 추가되어서인거 같습니다
고생하셨어여👍

Comment on lines +118 to +124
.warning-id {
margin: 6px 0 10px;
}

.warning-pw {
margin: 10px 0 13px;
}
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.

margin-left를 살짝 줘도 괜찮을꺼같아요:)

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.

오 한결 나은것 같네요! 🥹

Copy link
Copy Markdown
Collaborator

@merrybmc merrybmc left a comment

Choose a reason for hiding this comment

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

경민님 첫 pr을 축하드립니다. 🎉🎉
앞으로도 잘 해봐요 저희 😄

Comment on lines +55 to +66
<li class="icon-gl">
<a href="#">구글 계정으로 로그인</a>
</li>
<li class="icon-fb">
<a href="#">페이스북 계정으로 로그인</a>
</li>
<li class="icon-naver">
<a href="#">네이버 계정으로 로그인</a>
</li>
<li class="icon-kakao">
<a href="#">카카오톡 계정으로 로그인</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.

현재 cursor가 텍스트에만 먹히고 있고 로그인 버튼 전체에는 cursor가 먹히지 않고 있습니다.
그리고 시맨틱한 태그를 위해 로그인에는 단순한 페이지 이동이 아니기 때문에
a태그보단 button 태그가 더 잘 어울릴 것 같습니다.

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.

오호 시맨틱 태그에 더 신경을 써야겠네요.. 감사합니다! 😃

@AYFG
Copy link
Copy Markdown

AYFG commented Aug 4, 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