구현 과제 - 로그인/회원가입 모달 - 정승규 by Seunggyu008 · Pull Request #31 · FRONTENDSCHOOL7/CodeReviewStudy

Seunggyu008 · 2023-08-05T10:13:05Z

레이아웃 구현 완료하였습니다

#To do

커스텀 체크박스 스타일링

많이 부족하지만 잘 부탁드립니다!

merrybmc

안녕하세요 승규님. 첫 pr을 축하드립니다. 🎉🎉🎉🎉🎉

현재 구현된 사항에 대하여 체크박스에 대해 중점적으로 아쉬운 부분에 대해 코멘트를 남겨드렸습니다.
구현을 잘하셔서 피드백을 드릴 내용은 많이 없지만요.. 😢

칭찬해드릴 점은 특히 html 시맨틱 태그 부분은 많이 신경을 써주셔서 구현해주신 것 같습니다. 👍

merrybmc · 2023-08-07T11:36:49Z

정승규_login/css/login_page.css

input의 체크박스가 나타나고 있습니다.

{
overflow: hidden;
width: 1px;
height: 1px;
clip-path: inset(50%);
} 를 활용하여 체크박스를 숨겨주시는걸 권장해드립니다.

로그인상태유지 텍스트에 줄바꿈이 일어나고 있습니다.
태그의 css 코드 분리와 width값 등 수정이 필요해 보입니다.

아마 html에서 .a11y-hidden 요 클래스 설정을 안해주신거 같아요:)
{
overflow: hidden;
width: 1px;
height: 1px;
clip-path: inset(50%);
}

요걸로 처리하시면 될꺼같아요!

새로 커밋한 common.css파일에 a11y-hidden 클래스가 누락되어있었네요 ㅠ
줄바꿈도 체크박스가 나타나서 일어난거 같습니다.
다음엔 좀 더 꼼꼼하게 체크한 후 커밋하겠습니다!
피드백 주셔서 감사합니다 :))

merrybmc · 2023-08-07T11:41:50Z

정승규_login/login_page.html

웹접근성의 시맨틱 태그를 고려하였을 때 h1 태그를 쓰는 것이 좋아보여요. 😀

넵! 수정하겠습니다!

AYFG · 2023-08-07T16:26:48Z

안녕하세요 승규님!! 피그마에 나온 규격을 지키려 한 노력이 보이네요!! 고생 많으셨습니다 👍👍

::placeholder {
  position: absolute;
  top: 15px;
  bottom: 15px;
  left: 16px;

이 곳에 글씨가 어색하게 느껴졌는데 #user-pw에게 padding-left:15px이 이미 들어가있고 가운데 정렬이 되어있더라고요!
포지션 값을 준 4줄 한번 확인해주시면 좋을 것 같습니당!!😁
앞으로 잘부탁드립니다!!

YennieJ

승규님 고생하셨습니다 :)👍

사용하지 않는 default-font 클래스명은 지우는게 좋을꺼같아요
2.따로 설정하지 않는다면! font-style,height: nomal, font-weight: 400; 이 기본이라서 다 추가하지 않으셔도 됩니다 :)

YennieJ · 2023-08-07T07:20:33Z

정승규_login/index.html

div>a 라서 로그인 글씨 부분만 클릭이 되고 나머지 파란부분 영역은 클릭 되지 않습니다:)

YennieJ · 2023-08-07T18:12:50Z

af'

승규님 이 파일 뭐죠? :))

YennieJ · 2023-08-07T18:18:30Z

정승규_login/css/login_page.css

width는 굳이 주지 않아도 100%인데 사용하신 이유가 있을까요?

YennieJ · 2023-08-07T18:19:58Z

정승규_login/css/login_page.css

height: 100vh;
display: flex;
를 사용하셨으니
justify-content: center;
align-items: center;
사용하지 않고
.modal-container 에 margin: auto; 를 준다면
세로 사이즈가 줄었을 때 콘텐츠가 짤리지 않아요!

YennieJ · 2023-08-07T18:21:05Z

정승규_login/css/login_page.css

figma border width랑 값이 다른데 의도 하신건가요?:)

YennieJ · 2023-08-07T18:32:02Z

정승규_login/css/login_page.css

.modal-container img 너무 광범위해요!
header img 라던가,class 명을 추가하시는게 좋을 것같고
alt 명 또한 추가.
img태그 위치 조정이아니라 a태그 자체를 위치 조정하시는걸 추천드립니다!

#11

여기에 닫기 버튼 위치에 관련해서 글을 남긴게 있으니 참고해주시구요!

YennieJ · 2023-08-07T18:33:29Z

정승규_login/css/login_page.css

height는 자식요소 크기에 따라 달라지기 때문에 규정하지 않는것을 추천드립니다:)

YennieJ · 2023-08-07T18:40:50Z

정승규_login/login_page.html

span태그는 필요하지 않을꺼같아요
css는 .login-button에 합쳐주시면 됩니다!
button type은 submit입니다!
form 태그 안으로 옮겨서 로그인 버튼을 눌렀을 때 유저 정보가 보내질 수 있도록 해주세요:)

Seunggyu008

개선 사항:

a11y-hidden 누락 수정, 체크박스 줄바꿈 방지
placeholder 위치 수정
screen reader 기능을 고려안 x버튼 위치 조정

[#11] 로그인/회원가입 모달 에이아웃 구현

8b9adaf

Seunggyu008 added 리뷰요청 구현중 HTML/CSS labels Aug 5, 2023

체크박스 수정완료

76b0b99

Seunggyu008 added 구현완료 and removed 구현중 labels Aug 7, 2023

merrybmc reviewed Aug 7, 2023

View reviewed changes

merrybmc changed the title ~~구현과제 - 로그인/회원가입 모달 - 정승규~~ 구현 과제 - 로그인/회원가입 모달 - 정승규 Aug 7, 2023

YennieJ reviewed Aug 7, 2023

View reviewed changes

YennieJ force-pushed the 11-jungSeungGyu branch from fe21f5c to 76b0b99 Compare August 7, 2023 19:43

[#11]-로그인/회원가입 모달 코드 개선사항 반영 및 수정

d05d43f

Seunggyu008 commented Aug 8, 2023

View reviewed changes

Conversation

Seunggyu008 commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merrybmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AYFG commented Aug 7, 2023

Uh oh!

YennieJ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Seunggyu008 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Seunggyu008 commented Aug 5, 2023 •

edited

Loading