Skip to content

[정새론] sprint4 #130

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

Open
wants to merge 2 commits into
base: Basic-정새론
Choose a base branch
from

Conversation

Squarecat-meow
Copy link
Collaborator

요구사항

기본

로그인

  • 이메일 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 에 유효한 값을 입력하면 '회원가입' 버튼이 활성화 됩니다.
  • 활성화된 '회원가입' 버튼을 누르면 로그인 페이지로 이동합니다

심화

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

스크린샷

image
image

멘토에게

  • 코드가 굉장히 지저분하게 짜져있는 것 같은데, addEventListener를 계속해서 사용하는 게 아니라 어떤 추상화된 로직을 재사용할 수 있을 것 같은 기분이 듭니다. 예시가 있다면 몇가지 알려주시면 감사하겠습니다.

@Squarecat-meow Squarecat-meow added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Apr 29, 2025
Copy link
Collaborator

@addiescode-sj addiescode-sj left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!
js파일에 드린 코멘트 한번 참고해보시고, 리팩토링 진행해보시면 좋을것같아요 :)

주요 리뷰 포인트

  • 유지보수를 고려한 개발
  • 코드 간소화 및 중복 제거

</div>
</body>
<body>
<script src="./script/login.js" type="module"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

script 태그 위치도 브라우저 동작 원리를 이해하고계시다면 달라질수있답니다 :)
아래 아티클 참고로 읽어보세요!

참고

}
}

function passwordShowHide(button, input) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: 함수이름은 명사형보다는 동사로 시작하는게 좀 더 일반적입니다.
passwordShowHide => showHidePassword

네이밍 컨벤션 참고해볼까요?
참고

Comment on lines +5 to +11
if (checkValid) {
button.removeAttribute("disabled");
button.classList.add("login-button-enable");
} else {
button.setAttribute("disabled", true);
button.classList.remove("login-button-enable");
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

checkValid를 이런식으로 변수화하면 더 의미가 명확해질것같아요.

const isValid = validatorResults.every(Boolean);

그리고, classList.toggle을 사용하면 조건문을 사용하지않게되어 코드를 간소화할수있겠네요!

button.disabled = !checkValid;
button.classList.toggle('login-button-enable', isValid);

Comment on lines +21 to +23
if (input.getAttribute("type") === "password")
input.setAttribute("type", "text");
else input.setAttribute("type", "password");
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서도 조건문을 사용할 필요없이 둘중에 하나로 토글되는 함수라는걸 아니까, 간단하게 이렇게 바꿔주면 코드의 의도에 더 맞는 사용이 되겠죠?

    const isPasswordVisible = input.type === INPUT_TYPES.PASSWORD;

    input.type = isPasswordVisible ? 'text' : 'password';

Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 불필요한 DOM 접근이 많고, 유효성 검사 로직이 if 체이닝을 통해 복잡하게 중첩되어있어 가독성이 떨어지고, 이벤트 핸들러 내부에서 너무 많은 로직을 처리하고있어 유지보수에도 좋지 않을것같아요.

이 문제들을 해결하기위해 몇가지 리팩토링 제안을 드려볼게요!

우선 DOM 접근은 상단에서 한번만 하도록할까요?

const elements = {
  loginButton: document.getElementById("login-button"),
  emailInput: document.getElementById("email-input"),
  passwordInput: document.getElementById("password-input"),
  warningMessages: document.getElementsByClassName("warning-message"),
};

또, 상태 관리의 경우에도 객체로 한번에 관리하면 구조가 더 명확해질것같아요.

const state = {
  email: {
    isValid: false,
    isEmpty: true,
    isInvalid: false,
  },
  password: {
    isValid: false,
    isEmpty: true,
    isInvalid: false,
  },
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

그 다음, 하나의 함수가 너무 많은 일을 처리하기보다 한번에 하나의 일을 처리할수있게끔 단일 책임을 가진 함수 단위로 쪼개서 구조를 바꿔볼까요?

  • 유효성 검사
  • UI 업데이트
  • 이벤트 핸들러 연결

Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 이메일, 비밀번호에 대한 유효성 검사를 수행한다음 UI 업데이트를 하고있는데,
그렇다고 이런 일을 처리하는 개별 함수를 따로 다 만드는것보다는 재사용이 가능한 단위로 분리해보면 코드 중복도 줄어들고 훨씬 간결해질것같아요.

예를 들어 유효성 검사의 경우 UI와 결합될 필요가 있을까요?
유효성 검사 규칙과 수행은 프로그램내에서 어디서든 사용될수있고, UI 상태에 따라 변화하지도않아요.
따라서, 재사용을 고려해 아래와 같이 모듈화해보시는게 좋을것같아요.

// validation.js - 검증 로직
export const validators = {
  email: (value) => {
    if (!value) return { isValid: false, message: '이메일을 입력해주세요' };
    if (!/^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(value)) {
      return { isValid: false, message: '올바른 이메일 형식이 아닙니다' };
    }
    return { isValid: true, message: '' };
  },
  
  nickname: (value) => {
    if (!value) return { isValid: false, message: '닉네임을 입력해주세요' };
    return { isValid: true, message: '' };
  },
  
  password: (value) => {
    if (!value) return { isValid: false, message: '비밀번호를 입력해주세요' };
    if (value.length < 8) {
      return { isValid: false, message: '비밀번호는 8자 이상이어야 합니다' };
    }
    return { isValid: true, message: '' };
  },
  
  passwordCheck: (value, formValues) => {
    if (!value) return { isValid: true, message: '' }; // 빈 값은 허용
    if (value !== formValues.password) {
      return { isValid: false, message: '비밀번호가 일치하지 않습니다' };
    }
    return { isValid: true, message: '' };
  }
};

그러면 이벤트 핸들러 내부에서 유효성 검사를 수행하지않고 이런식으로 우리가 만든 validators를 재사용해줄 수 있어요.

  const { isValid, message } = validators.password(e.target.value);
  state.password.isValid = isValid;

Copy link
Collaborator

Choose a reason for hiding this comment

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

UI를 업데이트하는 작업 또한 함수로 만들고 인자로 에러상태를 반영해 클래스를 추가할 대상, 메세지를 추가할 대상, 유효한지 여부, 에러메시지를 전달해주면 반복되는 코드를 줄이고 우리가 원하는 작업을 잘 수행해줄수있게되겠죠? :)

// UI 업데이트 함수
const updateUI = (inputElement, messageElement, isValid, message) => {
  inputElement.classList.toggle("warning-input-border", !isValid);
  messageElement.textContent = message;
  messageElement.classList.toggle("warning-message-active", !isValid);
};

@addiescode-sj
Copy link
Collaborator

질문에 대한 답변

멘토에게

  • 코드가 굉장히 지저분하게 짜져있는 것 같은데, addEventListener를 계속해서 사용하는 게 아니라 어떤 추상화된 로직을 재사용할 수 있을 것 같은 기분이 듭니다. 예시가 있다면 몇가지 알려주시면 감사하겠습니다.

문제를 파악하는것부터가 문제 해결의 시작입니다.
스스로 느끼시고 개선해보시려는 마인드셋 아주 좋습니다 👍 본문 내에 자세히 코멘트 드려봤어요!

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