Skip to content

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

Open
kyokyoYa wants to merge 1 commit intomainfrom
11-hyunWoo
Open

구현 과제 - 로그인/회원가입 모달 - 현우#26
kyokyoYa wants to merge 1 commit intomainfrom
11-hyunWoo

Conversation

@kyokyoYa
Copy link
Copy Markdown

@kyokyoYa kyokyoYa commented Aug 2, 2023

로그인/회원가입 모달 과제 구현했습니다.

@kmryuuu
Copy link
Copy Markdown

kmryuuu commented Aug 3, 2023

'로그인 또는 회원가입' 부분에 창 닫는 아이콘이 없어서 추가해주시면 피그마와 동일하게 될 것 같아요! 😀

</head>
<body>
<div class="wrap">
<div class="header"><p>로그인 또는 회원가입</p></div>
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.

div태그안에 p태그를 사용하신 이유가 있을까요?
컨텐츠가 하나뿐이라 p태그를 사용하지 않아도 괜찮을꺼같아요:)

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.

이 부분은 시맨틱한 태그를 고려했을 때 header와 h1 태그를 써주면 더 좋을 것 같아요.

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.

좋은 피드백 감사합니다!

}

body {
padding-top: 5%;
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.

body의 컬러를 주는게 좋을것 같습니다:)

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 +111 to +116
#chk {
display: none;
width: 20px;
height: 20px;
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.

none을 사용하면 스크린 리더기가 읽을 수 없기 때문에 다른 방법으로 숨김 처리를 하는 것이 좋을꺼같요:)
참고
https://velog.io/@ursr0706/%EC%9B%B9-%EC%A0%91%EA%B7%BC%EC%84%B1%EC%9D%84-%EA%B3%A0%EB%A0%A4%ED%95%98%EC%97%AC-%ED%85%8D%EC%8A%A4%ED%8A%B8-%EC%88%A8%EA%B8%B0%EA%B8%B0

그리고 그외에 width,height,margin은 사용하지 않아도 같은 결과 값이 나옵니다:)

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.

아하 그렇군요! 좋은 피드백 감사합니다!

@YennieJ
Copy link
Copy Markdown
Collaborator

YennieJ commented Aug 3, 2023

고생하셨습니다 현우님:)

  1. CSS 변수를 정의하시거나 삭제하시는게 좋을것같습니다.
  2. 웹 접근성을 위한 시맨틱 태그에 대해 더 고민해보고 추가하시거나 변경하는걸 추천드립니다. ex) label
  3. font-weight,color,margin 확인이 필요해 보입니다.

큰 시간을 들이지않고 개발자도구를 사용하셔서 어떤 속성들이 어떻게 작동되는지 살펴보고 figma와 상관없이 필요하지 않은 CSS들은 삭제하는것이 좋을거 같습니다!

서로 코드를 보며 좋아보이는걸 쏙쏙 흡수합시다:))))

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 +127 to +142
.main1 label div {
position: relative;
width: 22px;
height: 22px;
background: url(./img/check-box2.png) no-repeat;
margin-right: 5px;
}

#chk:checked + label .box::after {
display: block;
content: "";
width: 22px;
height: 22px;
background: url(./img/check-box.png) no-repeat;
margin-right: 5px;
}
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
체크박스 svg 이미지
2.
image
현우님 체크박스 png 이미지

체크박스 이미지가 자글자글하게 깨지고 있습니다. svg 이미지 사용을 권장해드려요.

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.

매의 눈이시네요! 좋은 피드백 감사합니다!

</head>
<body>
<div class="wrap">
<div class="header"><p>로그인 또는 회원가입</p></div>
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.

이 부분은 시맨틱한 태그를 고려했을 때 header와 h1 태그를 써주면 더 좋을 것 같아요.

Comment on lines +13 to +23
<div class="main1">
<h1>위니브에서 여러분의 궁금증을 해결하세요! :)</h1>
<input type="text" placeholder="아이디"></input>
<p class="warning">아이디를 입력해 주세요</p>
<input type="password" placeholder="비밀번호"></input>
<p class="warning two">아이디 혹은 비밀번호가 일치하지 않습니다.</p>
<div class="check">
<input type="checkbox" id="chk">
<label for="chk"><div class="box"></div>로그인 상태 유지</label>
</div>
<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.

여기에 h2 태그를 써주시는 것과 시맨틱 태그와 기능까지 고려했을 때 form 태그로 묶어주면 훨씬 좋을 것 같아요.

참고자료: https://developer.mozilla.org/ko/docs/Web/HTML/Element/form

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.

form태그 적용 해보겠습니다!

Comment on lines +29 to +45
<div class="main2">
<a href="https://www.google.co.kr/?hl=ko"
><img src="./img/google.png" alt="구글" />
<p>구글 계정으로 로그인</p></a
>
<a href="https://www.google.co.kr/?hl=ko"
><img src="./img/facebook.png" alt="구글" />
<p>페이스북 계정으로 로그인</p></a
>
<a href="https://www.google.co.kr/?hl=ko"
><img src="./img/naver-logo.png" alt="구글" />
<p>네이버 계정으로 로그인</p></a
>
<a href="https://www.google.co.kr/?hl=ko"
><img src="./img/vector.png" alt="구글" />
<p>카카오톡 계정으로 로그인</p></a
>
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 태그보다 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.

아하 button이 맞군요!? 감사합니다!

@AYFG
Copy link
Copy Markdown

AYFG commented Aug 4, 2023

로그인 버튼에 letter-spacing을 주셨네요?! 로그인 버튼이 강조되게 보여서 이런 방법도 있구나 하면서 봤습니다~
고생하셨습니다👍👍

@timetam24
Copy link
Copy Markdown

안녕하세요 현우님!
클래스명, 변수명 등등 네이밍은 정말 머리 아픈 일인데요🫠

회원가입의 부모 div에 클래스명으로 membership을 주신 것처럼
.main button 보다 .login-button(예시)과 같이 의미가 담긴다면 가독성이 좋아져 유지보수가 필요할 때 더 편할 듯 합니다!

구현하느라 너무 고생 많으셨습니다!😊👍

@kyokyoYa
Copy link
Copy Markdown
Author

kyokyoYa commented Aug 6, 2023

좋은 피드백 감사합니다!!

@merrybmc merrybmc changed the title 구현과제 - 로그인/회원가입 모달 - 현우 구현 과제 - 로그인/회원가입 모달 - 현우 Aug 8, 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.

6 participants