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

refactor: next button 분기문 리팩토링 #104

Merged
merged 2 commits into from
Mar 2, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,29 @@ interface ApplicationNextButtonProps {
isLast?: boolean;
}

// etc. 단순히 boolean이 아닌 어느 곳에서 터지는 지와 그 이유를 담은 객체를 반환하면 어떨까?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 논의를 발제한 이유는, 아래 줄에(53L~55L) alert에 넘겨주는 문구를 조금 더 구체화 하고 싶어서 그랬습니다.
포트폴리오 동의여부에서 "동의하지 않습니다"를 선택해도 필수 항목을 입력해달라고 요청하는 게 플로우상 어색해 보인다고 생각했습니다.
물론 포트폴리오를 제출해놓고 동의하지 않는 사람은 없겠죠?ㅋㅋ

Copy link
Collaborator

Choose a reason for hiding this comment

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

음 이부분은 TF분들도 원하시는 것 같아요!

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

Choose a reason for hiding this comment

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

좋을 것 같아요!

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

Choose a reason for hiding this comment

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

일단은 그럼, 다른 이슈로 빼둘께요 !

// TODO: 질문의 이름마다 side effect가 있으니 주의하면 좋을 것
const canNext = (applicationName: Array<string>) => {
return applicationName.every((name) => {
const canNext = (applicationNames: Array<string>) => {
return applicationNames.every((name) => {
const EMPTY_STRING: string = "";
const localStorageValueFromName = localStorage.get(name, EMPTY_STRING);
Comment on lines +20 to +21
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아래 중복되서 사용되는 부분을 전부 삭제하고 상위로 끌어올렸어요.
리팩토링 과정에서는 별 문제 없어 보였습니다. 이 부분 확인 부탁드립니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

magic string을 제거한건 너무 감사하네요


if (
name === "personalInformationAgreeForPortfolio" ||
name === "personalInformationAgree"
) {
return (
localStorage.get(name) !== "동의하지 않습니다." &&
localStorage.get(name, "") !== ""
);
return localStorageValueFromName === "동의합니다.";
}
if (name === "email") {
return isEmail(localStorage.get(name, ""));
return isEmail(localStorageValueFromName);
}
if (name === "check") {
return localStorage.get(name) === "확인했습니다";
}
if (localStorage.get(name, "").length === 0) {
if (
name === "channel" &&
localStorage.get("channelEtc", "").length !== 0
) {
return true;
}
return false;
return localStorageValueFromName === "확인했습니다";
}
return true;
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

깔끔해졌네요.. 👍

!(localStorageValueFromName.length === 0) ||
(name === "channel" && localStorage.get("channelEtc", EMPTY_STRING).length !== 0)
);
});
};

Expand All @@ -57,7 +52,7 @@ const ApplicationNextButton = ({
);
if (!canNext(Array.from(applicationName))) {
alert("필수 항목을 입력해주세요.");
return false;
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이벤트 핸들러에서 boolean을 반환할 필요가 없다고 생각해 undefined으로 변경하였습니다.

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

@2yunseong 2yunseong Mar 2, 2024

Choose a reason for hiding this comment

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

혹시 몰라 더 찾아봤는데, 바닐라 자바스크립트에서는 이벤트핸들러가 false를 반환하면 기본 동작(이벤트 버블링)을 취소한다네요!
하지만 React에서는 지원하지 않는다고 합니다. (레거시 문서지만 현재버전까지 유지되는 내용 같아요 ! )

https://ko.legacy.reactjs.org/docs/events.html

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 습관적으로 저걸 반환하긴 했는데, react는 (0.14라면 꾀나 예전이네요 ㅋㅋㅋ) 안했었군요 !! ㅋㅋㅋㅋ

}

if (isLast) {
Expand Down