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

[김창민]week5 #344

Open
wants to merge 2 commits into
base: part1-김창민
Choose a base branch
from

Conversation

changmin6362
Copy link
Collaborator

@changmin6362 changmin6362 commented Jan 28, 2024

요구사항

기본

  • [x]- 회원가입 페이지의 이메일, 비밀번호, 비밀번호 확인 input에 필요한 유효성 검증 함수를 만들고 적용해 주세요.
  • [x]Github에 위클리 미션 PR을 만들어 주세요.
  • [x]React와 같은 UI 라이브러리를 사용하지 않고 진행합니다.
  • []

심화

  • [x]- 비밀번호를 확인할 수 있는 아이콘 클릭시 비밀번호의 문자열이 보이기도 하고, 가려지기도 합니다.
  • []로그인, 회원가입 페이지에 공통적으로 사용하는 로직이 있다면, 반복하지 않고 공통된 로직을 모듈로 분리해 사용해 주세요.

주요 변경사항

-에러 메시지를 class로 따로 떼어 낸 후 클래스를 추가하거나 제거하는 식으로 다루도록 바꿈
-innerHTML을 textContent로 바꿈
-구조화하는데 실패함(너무 많은 오류 발생)

스크린샷

image

멘토에게

-구조를 변경하려고 했는데 너무 많은 오류가 발생해서 포기했습니다
6시간정도 노력해봤는데 도저히 답을 찾지 못했습니다. 아무래도 로직이 잘못된것 같아서 다 갈아엎어야 할 것 같습니다

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@changmin6362 changmin6362 added 미완성 죄송합니다.. 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. labels Jan 28, 2024
@YunKukPark
Copy link
Collaborator

구조를 변경하려고 했는데 너무 많은 오류가 발생해서 포기했습니다

승구님의 PR을 보고 바로 창민님의 PR을 보는데 같은 어려움을 느끼시고 계시는군요! ㅎㅎ

  • 구조화 작업은 사실 많은 코드 경험이 있고, 요구사항을 보고 어떤 패턴을 사용해야 할지 알아야지만 자연스럽게 할 수 있는 단계가 됩니다. 하지만 창민님은 이미 로직을 잘 짜고 있기 때문에, 구조화에 대해 말씀드린 것은 창민님의 능력을 더 향상시키고자 하는 의도였어요.
  • 코드의 구조를 잡음으로써 변화에 대한 유연성을 제공한다는 것을 배워가시면 좋겠어요. 로직 중심으로 작업했다면 동일한 로직을 반복해서 사용하게 되고, 유지보수 시 놓칠 수 있는 부분이 생길 수 있습니다.
  • 처음에는 구조화가 어려울 수 있지만, 이 과정을 즐기며 '왜 이렇게 되지 않을까?', '어떻게 하면 다른 사람이 이 로직을 쉽게 이해할 수 있을까?'에 대해 고민하는 것이 중요해요. 이러한 과정이 코드를 단순히 작성하는 것이 아니라 설계하는 단계로 나아가는 데 도움이 될 거라고 생각합니다.
  • 갈아엎어야 한다는 생각을 하셨다면 왜 갈아엎어야 하는지에 대한 생각을 도출했는가가 너무 궁금해요. 갈아엎는다 라는 생각을 하는거면 더이상 이 로직과 구조를 다음 추가 개발에는 사용하지 못할 것 같다. 라는 생각에서 나올텐데 어떤 이유로 추가 개발에 사용하지 못할 것 같게 되었는지 생각의 흐름을 같이 공유해주시면 좋을 것 같아요!

Copy link
Collaborator

@YunKukPark YunKukPark left a comment

Choose a reason for hiding this comment

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

👍🏻 고생하셨습니다 창민님!
점점 더 좋아지는 코드가 눈에보이면서, 고민의 흔적도 보이네요!
다음 주차 더 성장한 창민님을 기대하도록 하겠습니다 👍🏻👍🏻👍🏻

Comment on lines +37 to +41
needPwdCorrect: {
validate: (value) => $passwordInput.value === value,
message: `비밀번호와 다릅니다`,
},
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

아마 이부분이 가장 큰 어려움이 있지 않을까 생각했어요

외부에서 값을 사용하는것보다 이렇게 사용해보는건 어때요?

Suggested change
needPwdCorrect: {
validate: (value) => $passwordInput.value === value,
message: `비밀번호와 다릅니다`,
},
};
needPwdCorrect: {
validate: ([compare, target]) => compare === target,
message: `비밀번호와 다릅니다`,
},
};

};

// input의 종류별로 행해질 유효성 검사를 배열로 구분한다
const validationMap = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻👍🏻 validator과 ruleMap을 signin과 signup모두 대응을 하기 위해 분리했군요 좋습니다

Comment on lines +51 to +55
$formInputList.forEach(($input) =>
$input.addEventListener("focusout", (event) => {
handleInputFocusout(event);
})
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

callback함수에서 보내주는 인자와 받는 인자가 같은 경우 생략이 가능합니다.

Suggested change
$formInputList.forEach(($input) =>
$input.addEventListener("focusout", (event) => {
handleInputFocusout(event);
})
);
$formInputList.forEach(($input) =>
$input.addEventListener("focusout", handleInputFocusout)
);


//key값 확인 후 ./folder/로 이동시킴
$loginForm.addEventListener("submit", (event) => {
if ($emailInput.value === keyEmail && $passwordInput.value === keyPassword) {
event.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

event의 기본 이벤트를 prevent하는건 if문 분기를 태우기 전에 실행되어야 할 것 같아요
틀린 값을 입력해보고 한번 시도해보세요!

@YunKukPark
Copy link
Collaborator

conflict resolve 부탁드려요! @changmin6362

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