Skip to content
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

[강성구] Sprint4 #110

Merged

Conversation

L1m3Kun
Copy link
Collaborator

@L1m3Kun L1m3Kun commented Jun 14, 2024

요구사항

기본 요구사항

  • Github에 PR(Pull Request)을 만들어서 미션을 제출합니다.
  • 피그마 디자인에 맞게 페이지를 만들어 주세요.
  • React와 같은 UI 라이브러리를 사용하지 않고 진행합니다.

체크리스트 [기본]

로그인

  • 이메일 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “이메일을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
  • 이메일 input에서 focus out 할 때, 이메일 형식에 맞지 않는 경우 input에 빨강색 테두리와 아래에 “잘못된 이메일 형식입니다” 빨강색 에러 메세지를 보입니다.
  • 비밀번호 input에서 focus out 할 때, 값이 없을 경우 아래에 “비밀번호를 입력해주세요.” 에러 메세지를 보입니다
  • 비밀번호 input에서 focus out 할 때, 값이 8자 미만일 경우 아래에 “비밀번호를 8자 이상 입력해주세요.” 에러 메세지를 보입니다.
  • input 에 빈 값이 있거나 에러 메세지가 있으면 ‘로그인’ 버튼은 비활성화 됩니다.
  • input 에 유효한 값을 입력하면 ‘로그인' 버튼이 활성화 됩니다.
  • 활성화된 ‘로그인’ 버튼을 누르면 “/items” 로 이동합니다

회원가입

  • 이메일 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “이메일을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
  • 이메일 input에서 focus out 할 때, 이메일 형식에 맞지 않는 경우 input에 빨강색 테두리와 아래에 “잘못된 이메일 형식입니다” 빨강색 에러 메세지를 보입니다.
  • 닉네임 input에서 focus out 할 때, 값이 없을 경우 input에 빨강색 테두리와 아래에 “닉네임을 입력해주세요.” 빨강색 에러 메세지를 보입니다.
  • 비밀번호 input에서 focus out 할 때, 값이 없을 경우 아래에 “비밀번호를 입력해주세요.” 에러 메세지를 보입니다
  • 비밀번호 input에서 focus out 할 때, 값이 8자 미만일 경우 아래에 “비밀번호를 8자 이상 입력해주세요.” 에러 메세지를 보입니다.
  • 비밀번호 input과 비밀번호 확인 input의 값이 다른 경우, 비밀번호 확인 input 아래에 “비밀번호가 일치하지 않습니다..” 에러 메세지를 보입니다.
  • input 에 빈 값이 있거나 에러 메세지가 있으면 ‘회원가입’ 버튼은 비활성화 됩니다.
  • input 에 유효한 값을 입력하면 ‘회원가입' 버튼이 활성화 됩니다.
  • 활성화된 ‘회원가입’ 버튼을 누르면 “/signup” 로 이동합니다

체크리스트 [심화]

  • 눈 모양 아이콘 클릭시 비밀번호의 문자열이 보이기도 하고, 가려지기도 합니다.
  • 비밀번호의 문자열이 가려질 때는 눈 모양 아이콘에는 사선이 그어져있고, 비밀번호의 문자열이 보일 때는 사선이 없는 눈 모양 아이콘이 보이도록 합니다.

스크린샷

loginAllowed loginError1 loginError2 loginVisibility signupAllowed signupError1 signupError2

사이트 주소

멘토에게

  • 리팩토링을 나름대로 했는데 피드백 부탁드립니다!
  • 보통은 어디까지 모듈화하는지 궁금합니다.

L1m3Kun added 19 commits June 12, 2024 15:37
previous - 상대 경로
current - url 태그 추가 및 절대 경로
1. 이메일 형식 검증
2. 비밀번호 형식 검증
3. 이메일, 비밀번호 검증에 따른 로그인 버튼 활성화
@L1m3Kun L1m3Kun added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jun 14, 2024
@L1m3Kun L1m3Kun requested a review from wlgns2223 June 14, 2024 02:59
Comment on lines +22 to +53
<form action="#" method="POST" id="signup-form">
<div class="email">
<label for="email">이메일</label>
<input type="email" name="email" placeholder="이메일을 입력해주세요">
<input type="email" name="email" id="email" placeholder="이메일을 입력해주세요">
<p class="error-message" id="email-error-message"></p>
</div>
<div class="email">
<label for="email">닉네임</label>
<input type="email" name="email" placeholder="닉네임을 입력해주세요">
<div class="nickname">
<label for="nickname">닉네임</label>
<input type="nickname" name="nickname" id="nickname" placeholder="닉네임을 입력해주세요">
<p class="error-message" id="nickname-error-message"></p>
</div>
<div class="password">
<label for="password">비밀번호</label>
<input type="password" id="password" name="password" placeholder="비밀번호를 입력해주세요">
<div class="hide-icon">
<div id="hidden"></div>
<div class="input-wrap">
<input type="password" id="password" name="password" placeholder="비밀번호를 입력해주세요">
<div class="visibility-icon">
<div id="visibility"></div>
</div>
</div>
<p class="error-message" id="password-error-message"></p>
</div>
<div class="password-confirm">
<label for="password-confirm">비밀번호 확인</label>
<input type="password" id="password-confirm" name="password-confirm" placeholder="비밀번호를 다시 한 번 입력해주세요">
<div class="hide-icon">
<div id="confirm-hidden"></div>
<div class="input-wrap">
<input type="password" id="password-confirm" name="password-confirm" placeholder="비밀번호를 다시 한 번 입력해주세요">
<div class="visibility-icon">
<div id="confirm-visibility"></div>
</div>
</div>
<p class="error-message" id="password-confirm-error-message"></p>
</div>
<button class="signin-btn">회원가입</button>
<button class="signup-btn" id="signup-btn" onclick="goOtherPage('login/login.html')" disabled>회원가입</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

form 내부 마크업 잘 되었어요 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다 :)

Comment on lines +47 to +49
<div class="visibility-icon">
<div id="confirm-visibility"></div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지 태그를 써도 되고, 배경 이미지로 해도 둘 다 맞는 방법이라고 생각합니다.
성구님은 image 태그가 아닌 왜 배경 이미지로 하기로 결정을 하셨죠?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

질문 감사합니다. 저도 고민을 많이 했는데 결국 다음과 같은 이유 때문인 것 같아요. 혹시 잘못된 부분있으면 피드백 주시면 감사하겠습니다 :)

  1. 초기 설정할 때, 눈 모양 아이콘사선이 그어진 아이콘을 모두 넣어두려 했었고, 이 과정에서 html로 표현하면 두 개의 태그를 사용해야한다고 생각했습니다.

  2. 당시 자바스크립트 진도가 나가지 않은 상태라 css로 구현하려 했고, 이 과정에서 image 태그로 구현하면 toggle에 있어 통일성이 떨어진다고 생각했습니다.

  3. 이 과정을 이전에는 :active를 통해 구현했으며, 현 과정에서 .active클래스를 통해 구현하는 방식으로 바꿨습니다.
    3-1. :active를 통해 구현하면 클릭을 누르고 있을 때만 이미지가 바뀌는 문제가 있었습니다.
    3-2. 자바스크립트 파일을 생성하고 로직을 추가하는 과정에서 .active 클래스를 이용하여 배경이미지를 바꾸는 방식으로 구현했습니다.

따라서 현재 image 태그가 아닌 배경 이미지를 통해서 구현하여 레이아웃을 그대로 두고, 배경 이미지만 다시 칠하는 방식을 생각하며 구현하였습니다.

지금 다시 생각해보면 image 태그의 속성 값만 바꿔줘서 렌더링시에 CSSOM을 건드리지 않는게 더 좋은 방식이라고 생각됩니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

리턴되는 값을 일치시켜주셔서 handleType의 리턴값으로 통일성있게 사용하고 계시네요. 좋은 아이디어라고 생각합니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다 :)

Comment on lines +6 to +7
const validation = new signupValidationState();
const password = new passwordState();
Copy link
Collaborator

Choose a reason for hiding this comment

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

클래스를 사용해주셨네요~ 구현을 보니까 상속까지 하셨던데
혹시 상속과 클래스를 사용하게 된 배경이 어떻게 될까요?

Copy link
Collaborator Author

@L1m3Kun L1m3Kun Jun 15, 2024

Choose a reason for hiding this comment

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

리뷰 감사합니다:)

리액트의 useState를 생각하며 비슷한 방식으로 만들어 보고싶었습니다.

함수로 구현하니 내부 값이 변동되지 않는 문제가 있어 클래스를 활용하였고, 로그인과 회원가입 간 state와 메서드가 겹치는 것이 있어 상속을 통해 구현하였습니다.


const handleErrorMessage = (inputType, errorType) => {
let errorMessage = '';
switch (errorType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

swtich문을 사용할때 default 절도 꼭 함께 사용해주세요. 핸들링되지 않는 case에 대해서 에러메세지를 던지거나 다른 처리를 함으로써 좀 더 안정적인 코드가 됩니다.

Copy link
Collaborator Author

@L1m3Kun L1m3Kun Jun 15, 2024

Choose a reason for hiding this comment

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

리뷰 감사합니다 :)

다음부턴 default도 고려하여 구현하겠습니다 :)

@wlgns2223
Copy link
Collaborator

리팩토링을 나름대로 했는데 피드백 부탁드립니다!

리뷰를 어떻게 할까 고민하다가 코드 하나하나는 잘 동작해가지고 로직을 작성하시는데는 무리가 없는것으로 판단이 되어서 코드 & 리팩토링을 어떻게 해야하는지에 대해 피드백을 드려야겠다고 생각했습니다.

우선 코드 작성과 관련해서 아시는지 모르겠지만 SOLID 원칙이라는 소프트웨어 개발에 중요한 원칙이 있습니다.
성구님의 코드 일부에서 SOLID원칙 중 첫번째 S에 해당하는 단일책임의 원칙을 위반한 코드를 발견 할 수 있습니다.
( SOLID원칙: https://inpa.tistory.com/entry/OOP-💠-객체-지향-설계의-5가지-원칙-SOLID )

코드작성

단일책임원칙: 객체는 하나의 책임만 가져야한다.
조금 바꾸면 함수는 하나의 책임만 가져야한다.

handleError({
    inputType:'이메일',
    btnName: 'signup',
    validation,
});
handleError({
    inputType:'닉네임',
    btnName: 'signup',
    validation,
});
handleError({
    inputType:'비밀번호',
    btnName: 'signup',
    validation,
    password,
});
handleError({
    inputType:'비밀번호 확인',
    btnName: 'signup',
    validation,
    password,
});

이 함수가 하는 일을 보시면 아래와 같이 4가지의 일을 합니다.

  1. 이메일을 검증하는 역할
  2. 닉네임을 검증하는 역할
  3. 패스워드를 검증하는 역할
  4. 패스워드 재입력값을 검증하는 역할

handleError가 4가지의 책임을 가지고 있으면 아무래도 한가지 타입의 인풋요소에 요구사항이 변화하면 handleError 함수의 수정이 어려워 질 수 있습니다. 또는 여러군데를 수정해야하는데, 이는 좋지 않은 징후입니다.

이럴 경우에는 각각 4가지의 함수로 분리를 하는것을 권장합니다.
handleErrorhandleEmailError, handleNicknameError, handlePasswordError등으로 나눕니다.
각 함수에서는 각각 인풋에 맞는 로직을 처리하면 됩니다. 저 같은 경우에도 각각의 인풋값에 대해 따로 함수를 작성하곤합니다.

리팩토링

아마 여기쯤 읽으시면 이제 의문점이 들수도 있고, 처음에 원래 그렇게 했다가 리팩토링을 해서 handleError 함수를 만드셨을 수도 있습니다.
의문점과 리팩토링을 하신 배경에는 제 생각에는 중복코드 제거를 위해 그랬을꺼라고 생각이 드는데요. 아니라면 혹시 리팩토링을 할때 어떤 목적을 가지고 하셨을까요?

의문점1. 각 인풋필드 마다 함수를 작성하면 함수를 여러번 적어야하는것 아닌가요?

함수가 많아지는것은 상관없다고 생각합니다. 읽기 쉽고 이해하기 쉬운 함수가 많아지는것은 나쁜 코드나 개발을 잘하거나 못하거나랑 관계가 없습니다.

의문점2. 솔직히 인풋필드마다 에러메세지 클래스를 toggle하는 로직이 비슷한데 합쳐야하는거 아닌가요?

사실 여기서부터 리팩토링을 시작해야합니다. 중복코드가 내가 정한 기준이상 등장한다면 한단계 추상화를 통해 공통로직을 뽑아내어 중복코드를 제거 할 수 있습니다. 오히려 리팩토링을 해야하는 대상은 마법같이 모든걸 처리하는 함수입니다. 리팩토링을 한 결과는 동작은 똑같은데 읽기 쉬워지고 단순하고 짧은 함수들이 만들어져야합니다.

중복코드는 일정수준의 중복코드는 사실 허용됩니다. 중복코드를 100% 없앨수는 없습니다. 다만 최대한 안 그럴려고 노력 할 뿐입니다.
저는 중복코드가 3번이상 등장하면 공통로직으로 뽑아내어 리팩토링을 하는 편입니다. 특별한 이유가 없고 함수 자체가 짧고 읽기 쉽고 다른 고민을 할 필요없이 의도가 이해가는 함수라면 리팩토링을 안합니다.

한번 handleError 함수를 쪼개서 각각의 함수 전용으로 로직을 작성해보세요 ㅎㅎ
궁금한점이 있다면 다음 멘토링때 질문을 준비해주세요 ~

@wlgns2223 wlgns2223 merged commit ab761a4 into codeit-bootcamp-frontend:Basic-강성구 Jun 14, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants