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

[1단계 - payments module] - 쿠키(김진호) 미션 제출합니다. #15

Merged
merged 84 commits into from
May 6, 2024

Conversation

jinhokim98
Copy link

@jinhokim98 jinhokim98 commented May 2, 2024

안녕하세요 유조! 6기 FE 리뷰이 쿠키입니다.
영화리뷰 때 상세한 리뷰 덕분에 많은 고민과 관점을 생각해 볼 수 있어서 감사했습니다.
이번에도 리뷰 잘 부탁드립니다.

배포주소

구조

이번에는 구조 설명을 npm readme에 적어두어서 링크로 대체하겠습니다.

🤔고민한 점과 질문

css library에 대한 고민

처음에 페어와 함께 모달을 만들면서 emotion, styled-components 중 어떤 것을 사용할까 고민했습니다.
하지만 이 프로그램을 사용하는 사용자가 일반 사용자가 아니라 개발자인 만큼 위 라이브러리를 사용함으로써 다른 사용자에게 위 스타일 라이브러리를 강제하게 되어버리는 것이 아닐까 해서 설치하지 않아도 되는 module.css 방식을 선택했습니다. 너무 과한 생각이었는지, 아니면 적절한 생각이었는지에 대한 유조의 의견이 궁금합니다!

custom hooks의 인터페이스에 대한 고민

이 역시 라이브러리를 사용하는 사용자가 개발자이기 때문에 이에 대해 고민이 많았습니다.
페어와 함께 인터페이스를 설계하면서 이번 미션의 훅들은 필드의 유효성을 검증하는 훅 이라고 명시가 되어있기 때문에 handleChange, handleBlur라고 적어도 유효성을 검사하는 기능을 포함하고 있다고 추측할 수 있다고 생각해서 이름을 지었습니다.
내부 사정을 모르는 사용자가 필드의 유효성을 검증하는 훅 이라는 기능을 보고 이 훅을 본다고 가정했을 때 사용자가 쉽게 예측을 할 수 있을지에 대해 고민입니다.

전반적으로 내부 구현을 모르는 사용자가 모달과 훅을 사용한다고 가정할 때 인터페이스만 보고도 어떻게 작동하는지, 예측이 가능한 지에 대한 피드백을 해주시면 감사하겠습니다👍👍

rbgksqkr and others added 30 commits April 30, 2024 14:11
@jinhokim98
Copy link
Author

jinhokim98 commented May 6, 2024

안녕하세요 유조! 즐거운 연휴 보내고 계신가요?
저는 이번 연휴를 조금 즐기느라 리뷰 반영이 늦었습니다..😥

이번 리팩터링에서 주로 챙긴 점은 커스텀 확대입니다.

모달 컴포넌트는 기존의 props로 모달 헤더, 컨텐츠, 푸터의 정보를 모두 내려주는 방식에서 합성 컴포넌트 방식으로 변경해서 각 컴포넌트에 맞는 props를 내려줄 수 있도록 변경해봤습니다.
그래서 모달의 헤더를 사용하지 않고 싶을 경우나, 헤더를 제일 아래에 내려서 사용할 수 있도록 확장성을 넓혀봤습니다.

훅의 경우에는 custom한 validation 로직을 외부에서 넣어줄 수 있도록 확장해봤습니다.
넣어주지 않을 경우 기존의 로직이 돌아가면서 외부에서 custom 로직을 넣어주면 custom 규칙대로 돌아가도록 설정해줘서, 좀 더 다양한 경우에도 사용할 수 있도록 개선해봤습니다!

이번에도 리뷰를 반영한 부분에는 커밋 번호를 달아두었고, 리뷰에 대한 제 의견은 리코멘트로 달아뒀습니다!

Copy link

@yujo11 yujo11 left a comment

Choose a reason for hiding this comment

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

안녕하세요 쿠키

연휴를 즐기셨군요 ㅋㅋㅋㅋㅋ 우테코 과정이 길기 때문에 잘 쉬는 것도 중요하다고 생각합니다 👍 👍 👍

기존 코멘트와 더불어 몇 가지 개선할만한 부분들 코멘트로 남겼는데 다음 step 진행하시면서 반영해보시면 좋을거 같습니다!

고생 많으셨습니다 쿠키 다음 step에서 만나요~~ 👍 👍 👍

@@ -6,6 +6,7 @@ module.exports = {
"plugin:@typescript-eslint/recommended",
"plugin:react-hooks/recommended",
"plugin:storybook/recommended",
"plugin:prettier/recommended",
Copy link

Choose a reason for hiding this comment

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

👍 👍 👍

Comment on lines +16 to +20
/* title typography */
--title-size: 18px;
--title-font-size: 18px;
--title-font-weight: 700;
--title-line-height: 26.06px;
Copy link

Choose a reason for hiding this comment

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

오 아주 좋네요 ㅎㅎ 👍 👍 👍 그런데 titls-sizetitle-font-size는 어떻게 다른걸까요? 🤔

Comment on lines +11 to +23
return (
<>
{!hide &&
(customButton ?? (
<button
className={`${styles.modalButton} ${styles.confirmButton}`}
{...props}
style={style}
>
{text ?? '확인'}
</button>
))}
</>
Copy link

Choose a reason for hiding this comment

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

이런 부분도 여러 조건이 JSX 내부에 섞이면 읽기가 어려워지는데요. 함수를 작성할 때처럼 early return 등을 적절하게 이용해보셔도 좋을거 같네요

Suggested change
return (
<>
{!hide &&
(customButton ?? (
<button
className={`${styles.modalButton} ${styles.confirmButton}`}
{...props}
style={style}
>
{text ?? '확인'}
</button>
))}
</>
if (hide || !customButton) return;
return (
<button
className={`${styles.modalButton} ${styles.confirmButton}`}
{...props}
style={style}
>
{text ?? '확인'}
</button>

Copy link

Choose a reason for hiding this comment

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

CloseButtonConfirmButton의 생김새가 매우 유사한데 공통되는 부분을 추상화해서 사용해보셔도 좋을거 같네용

Copy link

Choose a reason for hiding this comment

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

document 전체에 이벤트 리스너를 등록하는 대신에 Modal이 나올 때 Modal의 Background 영역을 같이 나오게 해서 해당 영역에 이벤트 리스너를 등록해서 핸들링 하는 방법도 있을거 같네요!

@@ -1,5 +1,5 @@
<!DOCTYPE html>
<html lang="en">
<html lang="ko">
Copy link

Choose a reason for hiding this comment

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

여기는 en이었군요 ㅋㅋㅋㅋ 꼼꼼하게 챙기시는 부분 좋습니다! 👍 👍 👍

@@ -13,7 +13,7 @@ npm i cookie-nice-modal
const App = () => {
return (
<Modal
isOpen={isOpen}
open={isOpen}
Copy link

Choose a reason for hiding this comment

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

👍 👍 👍

Comment on lines 31 to 34
const MODAL_TYPE: Record<ModalType, string> = {
dialog: styles.dialog,
drawer: styles.drawer,
};
Copy link

Choose a reason for hiding this comment

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

명칭 좋네요 👍 👍 👍

@yujo11 yujo11 merged commit bd1075d into woowacourse:jinhokim98 May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants