[2단계 - 페이먼츠 모듈] 에프이(박철민) 미션 제출합니다.#68
Conversation
components/src/lib/Modal/Modal.tsx
Outdated
| const modalWidth = (): CSSProperties['width'] => { | ||
| if (modalSize) { | ||
| switch (modalSize.width) { | ||
| case 'small': | ||
| return '320px'; | ||
| case 'medium': | ||
| return '480px'; | ||
| case 'large': | ||
| return '600px'; | ||
| default: | ||
| return modalSize.width; | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
사용자가 직접 크기를 지정할 수도 있고, 위의 세 가지 literal을 사용할 수도 있습니다.
편의성이 좋네요 👍
components/src/lib/Modal/Modal.tsx
Outdated
| if (modalSize) { | ||
| switch (modalSize.width) { |
There was a problem hiding this comment.
modalSize?.width 옵셔널 체이닝으로 depth를 줄여보면 어떨까요? 혹은 modalSize가 지정되지 않은 경우 반환값을 더 명시적으로 표현할 수 있는 early return도 좋을 것 같습니다 :)
There was a problem hiding this comment.
width:
${(modalSize && modalWidth()) || (modalPosition === 'center' ? '80%' : '100%')},
이 부분을 보고 modalWidth(modalPosition)이렇게 인자를 받아서 항상 undefined가 아닌 값만 반환하도록 하는 방법도 생각났습니다.
(다시보니 modalSize가 있을 때만 modalWidth를 실행하는 것 같네용)
There was a problem hiding this comment.
modalWidth가 실행되는 조건은 modalSize가 있는 경우입니다! 그래도 modalWidth 함수 내부에서는 modalSize가 존재 여부를 인식하지 못하기 때문에, modalSize가 없는 경우에는 early return하도록 수정했습니다 :)
const modalWidth = (): CSSProperties['width'] | undefined => {
if (!modalSize) return;
switch (modalSize.width) {
case 'small':
return '320px';
case 'medium':
return '480px';
case 'large':
return '600px';
default:
return modalSize.width;
}
};There was a problem hiding this comment.
modalWidth가 실행되는 조건은 modalSize가 있는 경우입니다
그렇다면 인자로 받고 타입을 더 명확히 표현해도 좋을 것 같습니다 :)
const modalWidth = (size: SizeProps): CSSProperties['width'] => {
switch (size.width) {
case 'small':
return '320px';
case 'medium':
return '480px';
case 'large':
return '600px';
default:
return size.width;
}
};
components/src/lib/Modal/Modal.tsx
Outdated
| <Modal.Header {...modalHeader} /> | ||
| <Modal.Content {...modalContent} /> | ||
| <Modal.Footer {...modalFooter} /> | ||
| <ModalHeader {...modalHeader} /> | ||
| <ModalContent {...modalContent} /> | ||
| <ModalFooter {...modalFooter} /> |
There was a problem hiding this comment.
<div>
<section>
{children}
</section>
</div>아하 음, 변경 전 합성컴포넌트 방식일 때 위처럼 쓰인 것이 아니라
props 방식처럼 ModalMain에서 직접 Modal.Header를 넣어주는 방식으로 작성해주셨었군용
그래서 합성컴포넌트 해제 전후로 큰 차이가 두드러지지 않는 것 같아요.
There was a problem hiding this comment.
모달 구현 방식, props vs 합성 컴포넌트
모달 컴포넌트를 설계할 때, 각 방식의 장단점을 이미 잘 파악하고 계신 것 같아요 👍
제 개인적인 의견으로는 현재 props의 개수로는 현재 구현해주신 방식을 유지해도 좋다고 생각합니다.
컴포넌트의 복잡도가 크지 않다면 에프이가 말씀해주신 것처럼 props 방식이 좋아보입니다. 특히 구성요소가 비교적 단순하거나 커스터마이징의 필요성이 낮은 경우, 개발하는 입장에서도 더 빠르게 구현할 수 있어 보여요.
또, 에프이가 해주신 것처럼 계층을 나누어 prop을 구성한다면, (꼭 합성컴포넌트로 구현하지 않아도) 충분히 관심사의 분리를 할 수 있어보입니다.
interface ModalProps {
modalHeader: ModalHeaderProps;
modalContent?: ModalContentProps;
modalFooter?: ModalFooterProps;
...
}합성 컴포넌트를 사용하면 사용자가 모달의 구조를 직접 결정할 수 있다는 점이 좋은 것 같습니다. 하지만 Header, Content, Footer로 나뉘어있는 모달 내부를 일일이 채우고 스타일링해야 하기 때문에 필요 이상의 힘이 들어갈 것 같습니다.
동의합니다. 그리고 이 부분은 충분한 예제 제공으로 커버할 수 있다고 생각합니다. 그러면 사용자가 바닥부터 코드를 작성해야하는 일은 거의 없을 것 같아요.
한편, 합성컴포넌트는 각 컴포넌트 구조를 명확하게 "선언"하고, 각 부분의 구성이 어떻게 이루어져 있는지 직관적으로 이해할 수 있도록 도와주는 측면도 있습니다. 이런 측면에서도 복잡도가 높을수록 추천드리고 싶은 방식입니다.
<Modal>
<Modal.Header />
<Modal.Boday />
<Modal.Footer />
</Modal>There was a problem hiding this comment.
그래서 합성컴포넌트 해제 전후로 큰 차이가 두드러지지 않는 것 같아요.
맞습니다😅 props 방식으로 쭉 구현하다가 합성 컴포넌트를 중간에 적용했더니, 전체 구조를 다 바꾸지 않는 이상 그 이점을 제대로 활용할 수 없었어요.
단순히 컴포넌트의 이름이 바뀌는 정도였던 것 같습니다.
There was a problem hiding this comment.
자세한 설명 감사합니다! 제가 놓치고 있던 부분도 함께 챙길 수 있었어요. 합성 컴포넌트를 사용하더라도 예제를 제공한다면 손쉽게 구현 가능하겠네요!
| cardNumber: 4, | ||
| expiryMonth: 2, | ||
| expiryYear: 2, | ||
| export type CardBrand = 'Visa' | 'Mastercard' | 'Diners' | 'AMEX' | 'Unionpay' | 'None'; |
There was a problem hiding this comment.
사소한데 타입 리터럴을 재선언하지 않고 keyof를 활용해보면 어떨까요?
There was a problem hiding this comment.
조금 더 힌트를 듣고 싶습니다 하루!
export const META_CARD: Record<CardBrand, { maxLength: number; format: number[] }> = {
Visa: { maxLength: 16, format: [4, 4, 4, 4] },
Mastercard: { maxLength: 16, format: [4, 4, 4, 4] },
Diners: { maxLength: 14, format: [4, 6, 4] },
AMEX: { maxLength: 15, format: [4, 6, 5] },
Unionpay: { maxLength: 16, format: [4, 4, 4, 4] },
None: { maxLength: 16, format: [4, 4, 4, 4] },
};바로 밑에서 정의한 META_CARD의 경우 CardBrand로부터 파생된 상수이고, useCardNumber에서 반환하는 카드 브랜드의 타입 역시 CardBrand를 사용하고 있습니다.
| mastercard: /^5[1-5]\d*/, | ||
| diners: /^36\d*/, | ||
| amex: /^3[4|7]\d*/, | ||
| unionpay: /^62(212[6-9]|21[3-9][0-9]|2[2-8][0-9][0-9]|29[0-1][1-9]|292[0-5]|[4-6]|8[2-8])\d*/, |
There was a problem hiding this comment.
정규표현식의 사용
카드 브랜드를 식별하기 위해 정규표현식을 사용했습니다. 상수로 분리했기 때문에 사용자는 구체적인 정규표현식을 보지 않아도 되고, 코드가 간단해지기 때문에 선호하는 편입니다. 하지만 Unionpay의 규칙 정도 되니 정규 표현식 자체가 길고 복잡해진다는 느낌을 받았습니다. 하루는 어떤 방법을 선호하시나요?
아하 음 카드브랜드 판별 로직은 빈번하게 변경이 발생할 것 같지 않아, 저라면 제가 빠르게 구현하기 편한 방향으로할 것 같습니다. 저는 제게 정규식보다 더 편한 TypeScript 코드로 작성할 것 같아요. 혹은 정규표현식으로 작성하고, 주석으로 정책 내용을 표현해둘 것 같습니다.
그리고 사용 여부를 판단하는 기준에 있어 개발자의 유지보수성을 1순위로 고려하는 것이 적절할까요?
좋은 의견이라고 생각합니다. 유지보수성과 함께 구현편의성도 고려하면 좋을 것 같습니다. 그 외로 (특히 복잡하고 긴 정규 표현식에서 그룹화, 캡처 그룹, 백트래킹 등이 많이 발생할 경우) 성능 이슈를 논하기도 합니다.
There was a problem hiding this comment.
혹은 정규표현식으로 작성하고, 주석으로 정책 내용을 표현해둘 것 같습니다.
주석으로 정책을 표현하는 방법도 굉장히 좋은 것 같습니다! 시작 번호의 최솟값과 최댓값 등을 상수로 정의한다면 별다른 설명 없이도 이해할 수 있겠지만, 정규 표현식은 그러기 힘드니 주석을 달아주는 것이 더 좋을 것 같아요.
사용 여부 판단 기준 관련 답변 주신 부분에서 정규 표현식의 성능이 저하되는 경우를 말씀해주셨는데, 해당 부분은 추가적으로 공부를 해보려 합니다! 단순히 긴 로직을 한 문장으로 정리 가능하다 정도로만 생각하고 지금까지 사용해왔는데, 성능 측면에서도 함께 생각을 해보아야 할 것 같아요.
감사합니다!
hooks/src/lib/useCardType.ts
Outdated
| const useCardType = () => { | ||
| const handleCardTypeChange = (value: string) => { | ||
| if (REGEX.visa.test(value)) return 'Visa'; | ||
| else if (REGEX.mastercard.test(value)) return 'Mastercard'; | ||
| else if (REGEX.diners.test(value)) return 'Diners'; | ||
| else if (REGEX.amex.test(value)) return 'AMEX'; | ||
| else if (REGEX.unionpay.test(value)) return 'Unionpay'; | ||
| else return 'None'; | ||
| }; | ||
|
|
||
| return { handleCardTypeChange }; | ||
| }; |
There was a problem hiding this comment.
이 useCardType 함수에는 아무런 use-*가 사용되지 않아, 훅이 아닌 단순한 함수로 작성해주시는 것이 더 적절해보입니다.
There was a problem hiding this comment.
커스텀 훅 내부에서 관리하고 있는 상태나 다른 훅이 없어서 유틸 함수로 분리하는 것이 적절한 것 같습니다.
기존에는 useCardType 내부에서 카드 브랜드를 상태로 관리하고 이를 useCardNumber 내부에서 사용했습니다. 그러다 보니 카드 번호 유효성 검사를 할 때 이전 브랜드를 가지고 검사하기 때문에 화면에 한 박자씩 느리게 출력되었어요. 이 문제를 해결하기 위해 useEffect를 사용할 수도 있었지만, cardType 자체를 상태로 가지지 않고 일반 함수로 즉시 반환하는 방법을 적용했습니다!
| - 입력한 카드 번호에 맞는 카드 브랜드를 반환합니다. | ||
| - 카드 브랜드에 맞는 카드 번호 자릿수를 제한합니다. | ||
| - 숫자 이외의 입력을 차단합니다. | ||
| - 카드 브랜드마다 다른 포맷팅 규칙을 적용해서 배열 형태로 반환합니다. `join`을 통해 사용하세요. |
There was a problem hiding this comment.
이렇게 사용법을 제시한 경우, 예시가 함께 제공되어도 좋겠습니다. (join 해서 사용한 예시. 이번에 반영해주시지 않아도 좋습니다.)
There was a problem hiding this comment.
사용법 안내 조금 밑에 join을 사용한 예제가 있습니다!
사용 예시:
const { cardNumberInfo, handleCardNumberChange } = useCardNumber();
{
return (
<input
type='text'
value={cardNumberInfo.cardNumber.join(' ')}
onChange={(e) => handleCardNumberInputChange(e.target.value)}
/>
);
}
components/src/lib/index.ts
Outdated
| export { default as Modal } from './Modal/Modal'; | ||
| export { default as AlertModal } from './AlertModal/AlertModal'; | ||
| export { default as ConfirmMoal } from './ConfirmModal/ConfirmModal'; | ||
| export { default as ConfirmModal } from './ConfirmModal/ConfirmModal'; |
There was a problem hiding this comment.
VS Code 익스텐젼 Code Spell Checker 추천드립니다 :)
There was a problem hiding this comment.
지금까지 code spell checker를 사용하지 않았는데, 이번 기회에 사용해봐야겠어요. 감사합니다!
| layout: 'fullscreen', | ||
| }, | ||
|
|
||
| tags: ['autodocs'], |
There was a problem hiding this comment.
모든 스토리에서 autodocs를 사용할 상황이라면,
새로 추가하는 stories.tsx 마다 'autodocs'를 설정하지 않고 일괄적으로 설정해봐도 좋을 것 같습니다.
There was a problem hiding this comment.
main.ts와 preview.ts를 수정해서 documentation을 자동으로 생성하게 했습니다!
| expect(result.current.handleCardTypeChange('3789')).toBe('AMEX'); | ||
| }); | ||
|
|
||
| it("622126~622925 사이의 숫자로 시작하는 번호를 입력하면 'Unionpay'를 반환한다.", () => { |
There was a problem hiding this comment.
감사합니다 하루! 테스트 이름과 그 코드만으로 기능 명세 역할을 할 수 있게 작성해보았어요. 😊
| const handleClose = () => { | ||
| setIsOpen(false); | ||
| args.closeButton.onClose(); | ||
| }; |
There was a problem hiding this comment.
'확인' 버튼을 클릭해도 모달이 닫히는 동작을 확인할 수 없어서 이 부분 확인 부탁드립니다 :)
There was a problem hiding this comment.
세 가지 커스텀 모달(Alert, Confirm, Prompt)에 대해 버튼을 누르면 모달이 닫히도록 수정했습니다!
| }, | ||
| }; | ||
|
|
||
| export const SmallSize: Story = { |
There was a problem hiding this comment.
modalPosition이 bottom인 경우의 스타일링이 제대로 설정되지 않아서 발생한 문제입니다! 간단하게 스타일을 주어서 해결했습니다 :)
365kim
left a comment
There was a problem hiding this comment.
에프이, 안녕하세요
2단계 작업해주신 것 잘봤습니다.
2단계 미션 요구사항을 더 잘 수행하기 위해서 다양한 것을 시도하고 또 깊이 고민해주신 것으로 보여요. 감기로 컨디션이 좋지 않으셨을텐데 너무 고생하셨습니다. 🙏 상세한 설명과 커밋 히스토리를 통해 에프이의 고민 내용을 자세히 알 수 있어서 좋았습니다 :)
몇 가지 코멘트 남겼는데 천천히 확인해보시고 반영 완료되시면 리뷰 재요청 부탁드릴게요.
수고하셨습니다 👏💊👏💊👏
|
안녕하세요 하루! 피드백 반영을 마치고 돌아왔습니다. 다음 미션과 병행하다 보니 충분한 시간을 투자하지 못했습니다😭 그래서 일부 피드백(TsDoc 적용)은 아직 적용하지 못한 상황입니다! 오늘 세 번째 미션을 제출하고 자투리 시간을 활용해서 마저 적용해보려 합니다. 🌐배포 주소🔧수정 사항스토리북 보완
useCardType 제거
❓질문
|
| children: ( | ||
| <> | ||
| <ModalInput onChange={(e) => console.log(e.target.value)} /> | ||
| <Input onChange={(e) => console.log(e.target.value)} /> |
| }, | ||
| docs: { | ||
| autodocs: "tag", | ||
| autodocs: true, |
hooks/src/lib/useCardNumber.ts
Outdated
| import useCardType from './useCardType'; | ||
| import { formatCardNumber } from './../utils/formatCardNumber'; | ||
| import { checkCardType } from './../utils/checkCardType'; |
365kim
left a comment
There was a problem hiding this comment.
에프이, 페이먼츠 모듈 미션 완료 축하드립니다 🎉
새로운 미션이 시작되어서 일정 관리가 어려우셨을 텐데도 피드백도 적극적으로 반영해주시고 끝까지 열정적으로 임해주셔서 감사합니다 💯
다음 미션도 응원할게요 💪🚀


안녕하세요 하루! 2단계 미션을 진행했습니다.
1단계 제출 이후부터 일주일째 감기가 낫질 않고 있어서 쉬는 것에 집중하다 보니 제출이 조금 늦어졌네요🤒 감기 조심하세요!
페이먼츠 미션에 적용하는 것은 조금 천천히 진행해보겠습니다😅
🌐배포 주소
모달 패키지 바로가기
모달 스토리북 바로가기
카드 커스텀 훅 패키지 바로가기
📚구조
모달
카드 훅
useCardNumberValidation→useCardNumber)❗기능 구현 사항
모달
Modal을 사용한 세 종류의 커스텀 모달 컴포넌트를 제공합니다.AlertModal- 메시지 + 확인 버튼을 띄웁니다.childrenprop을 받지 않습니다.ConfirmModal- 메시지와 함께 확인/취소 버튼을 띄웁니다.PromptModal- 입력을 받을 수 있는 input 필드와 확인/취소 버튼을 띄웁니다.childrenprop을 받지 않습니다.Modal컴포넌트를 사용해서 구현할 수 있습니다.small,medium,large를 지원합니다.modalSize의width속성으로 설정할 수 있습니다.카드 훅
카드 브랜드를 식별하는 로직을 추가했습니다.❓질문
정규표현식의 사용
모달 구현 방식, props vs 합성 컴포넌트
Header,Content,Footer로 나뉘어있는 모달 내부를 일일이 채우고 스타일링해야 하기 때문에 필요 이상의 힘이 들어갈 것 같습니다.