-
Notifications
You must be signed in to change notification settings - Fork 208
[1단계 - 페이먼츠] - 하쿠(이동현) 미션 제출합니다. #432
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
Conversation
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
Co-authored-by: yeongipark <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a credit card payment UI with reusable input components, real‐time preview updates, and corresponding validations. Key changes include the creation of styled Input components and their Storybook stories, separated sections for card number, expiration period, and CVC inputs, and integration with Chromatic for visual testing.
Reviewed Changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/input/Input.tsx | Created a reusable Input component with error styling. |
| src/components/input/Input.stories.tsx | Added Storybook stories for the Input component. |
| src/components/cardPreview/CardPreview.tsx | Developed a CardPreview component that identifies card logos. |
| src/components/cardNumberSection/CardNumberSection.tsx | Added a section to collect card number input with title. |
| src/components/cardNumberInputs/CardNumberInputs.tsx | Implemented card number inputs with inline validation. |
| src/components/cardExpirationPeriodSection/CardExpirationPeriodSection.tsx | Added section for expiration period input with validation. |
| src/components/cardExpirationPeriodInputs/CardExpirationPeriodInputs.tsx | Created expiration period inputs and validations. |
| src/components/cardCVCNumberSection/CardCVCNumberSection.tsx | Developed a section for CVC input with its validations. |
| src/components/cardCVCNumberInputs/CardCVCNumberInputs.tsx | Implemented CVC input component and its Storybook stories. |
| src/App.tsx | Integrated all sections and preview components into the main app. |
| src/�types/index.types.ts | Defined types for card number, expiration period, and CVC. |
| README.md | Updated documentation with implementation details. |
| .github/workflows/chromatic.yml | Added workflow configuration for Chromatic visual tests. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (2)
src/components/cardExpirationPeriodInputs/CardExpirationPeriodInputs.tsx:9
- The constant EXPIRATION_PERIOD_LENGTH is set to 3 while the placeholders and error messages indicate a 2-digit input requirement. Consider updating it to a value of 2 to match the intended validation.
const EXPIRATION_PERIOD_LENGTH = 3;
src/components/cardPreview/CardPreview.tsx:2
- The import path contains an unexpected '\btypes' segment. Verify that the file path is correct and remove any stray characters to ensure proper module resolution.
import { CardNumber, ExpirationPeriod } from "../../\btypes/index.types"
yujo11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 하쿠 ~~ 만나서 반갑습니다 ~~ 잘 부탁드려요~~
전체적으로 코드를 짜임새 있게 잘 구성하셨다는 느낌을 받았는데요. 코드를 보며 궁금한 점들과 개선시켜볼만한 부분들에 대해 코멘트 남겨놨으니 확인해보시면 좋을거 같네요 ㅎㅎ
CSS 스타일을 적용하는 3가지 방식에 각각 장단점은 확인하였습니다. 여기서 저희 페어만의 기준을 세우고 styled component를 선택하였으나 유조만에 스타일을 적용하는 방식을 선택하는 기준에 대한 방향성이 어떤지 궁금합니다!
프로젝트의 규모와 투입되는 인원 등에 따라 다르게 선택되는 부분인데요. 프로젝트의 런타임 오버헤드나 번들 크기를 신경써야 하는지 등 다양한 요소들이 고려될거 같아요.
왜 Styled Components를 선택하셨는지 잘 설명해주셔서 좋았는데요. 말씀해주신 단점 외에도 CSS in JS 방식이 가지는 단점들이 있는데 여기에 대해서도 더 찾아보시면 좋을거 같습니다 ㅎㅎ
(저는 개인적으로 tailwind 좋아합니다)
미션 진행하느라 고생 많으셨습니다 하쿠 ~~ 👍 👍 👍
| "react": "^19.1.0", | ||
| "react-dom": "^19.1.0" | ||
| "react-dom": "^19.1.0", | ||
| "styled-components": "^6.1.17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
선택에 대한 이유를 잘 설명해주셔서 좋았네요 ㅎㅎ 👍 👍 👍
src/types/index.types.ts
Outdated
| export type CardCVCNumberSectionProps = { | ||
| CVCNumber: string, | ||
| changeCVCNumber : (CVCNumber : string) => void | ||
| } No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/types/index.types.ts
Outdated
|
|
||
| export type ExpirationPeriodProps = { | ||
| expirationPeriod: ExpirationPeriod | ||
| changeExpirationPeriod: (expirationPeriod: keyof ExpirationPeriod, date: string) => void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
타입 시스템을 잘 활용하고 계시는군요 👍 👍 👍
src/App.tsx
Outdated
| const StyledApp = styled.div` | ||
| display: flex; | ||
| justify-content : center; | ||
| ` | ||
|
|
||
|
|
||
| const StyledFrame = styled.div` | ||
| display: inline-flex; | ||
| padding: 77px 30px 19px 31px; | ||
| flex-direction: column; | ||
| justify-content: flex-end; | ||
| align-items : center; | ||
| gap: 45px; | ||
| background-color: white; | ||
| width: 100%; | ||
| max-width: 600px; | ||
| box-sizing: border-box; | ||
| ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style 파일을 분리하지 않고 한 파일 내에 작성하신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공통적으로 여러곳에서 쓰는 style은 별도의 파일로 만들었지만, 그러지 않은 경우에는 tsx 파일에 내포하여 UI 상으로 해당 dom이 어떤 요소인지 직관적으로 확인하고자 넣었습니다!
src/App.tsx
Outdated
| type CardNumberState = { | ||
| [key in Position]: string; | ||
| }; | ||
|
|
||
| type ExpirationPeriodState = { | ||
| [key in keyof ExpirationPeriod]: string; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍
| if (!isValidLength(number, length)) { | ||
| setError((prev) => { | ||
| prev[position] = errorMessage.length | ||
| return {...prev} | ||
| }) | ||
| return; | ||
| } else if (!isValidNumber(number)) { | ||
| setError((prev) => { | ||
| prev[position] = errorMessage.number | ||
| return {...prev} | ||
| }) | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기는 else-if 문을 사용하는데 하쿠의 사용 기준이 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리팩터링 과정에서 해당 부분의 else if문을 바꾼다는 것을 빼먹었습니다. 수정하겠습니다!
| [POSITION.FOURTH]: NO_ERROR, | ||
| }); | ||
|
|
||
| function checkValidation(position: Position, length: number, number: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여러개의 인자를 받을 때는 객체로 인자를 받는 것도 고려해보시면 좋겠네요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네! 고민해보겠습니다!
| @@ -0,0 +1,51 @@ | |||
| import styled from "styled-components"; | |||
|
|
|||
| type InputProps = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 컴포넌트는 앞으로 전체 애플리케이션에서 html의 input element를 대신해서 사용될거 같은데요. 그렇다면 html input이 받을 수 있는 attribute들을 모두 사용 할 수 있게 만든다면 보다 유연한 사용이 가능해질거 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 당장 사용하지 않는 attribute도 넣어놓는 것이 맞다는 말씀이실까요? 우선 범용적으로 쓸수 있게 만들어놓고, 나중에 필요한 attribute를 사용할 때 추가하는게 좋지 않을까 생각했습니다
| </StyledMagnetic> | ||
| {logoSrc !== INITIALIZE_VALUE ? | ||
| <StyledLogoWrap> | ||
| <img src={logoSrc} alt='logo' style={{width: '100%', height: '100%'}}/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alt 태그도 놓치지 않고 잘 넣어주셨군요 ㅎㅎ 👍 👍 👍
| function identifyLogo() { | ||
| const id = cardNumber['first'].slice(0, 2); | ||
| if (Number(id[0]) === CARD_IDENTIFYING_NUMBER.VISA) setLogoSrc('./images/Visa.svg') | ||
| else if (Number(id) >= CARD_IDENTIFYING_NUMBER.MASTERCARD.MIN && Number(id) <= CARD_IDENTIFYING_NUMBER.MASTERCARD.MAX) setLogoSrc('./images/Mastercard.svg') | ||
| else { setLogoSrc(INITIALIZE_VALUE) } | ||
| } | ||
| identifyLogo(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수를 useEffect 안에서 선언 후 사용하는 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
따로 고려는 안했지만 메모리 낭비가 될 수도 있겠다라는 생각이 있어 useEffect를 안쓰는 방향으로 리팩터링 예정입니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조건에 따라 로고를 출력하는 형식으로 변경하였습니다!
9ed0df5
|
지금은 스토리북 잘 들어가지네요! |
yujo11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 하쿠 ~~
리뷰들 꼼꼼하게 확인해주셔서 감사합니다 ㅎㅎ
코드 작성 배경에 대한 질문을 드렸는데 뚜렷한 이유 없이 작성 된 코드가 많은거 같아 조금 아쉽게 느껴졌어요. 코드를 작성하다 보면 정답이 없는 부분들이 많기 때문에 코드를 이해할 때 코드를 작성한 작성자의 의도를 알게 된다면 더 쉽게 이해가 가능한데요. 더불어 리뷰 과정에서는 이런 배경과 의도를 설명해주신다면 더 효과적인 피드백을 드릴 수 있을거 같습니다.
앞으로 코드를 작성하실 때 그냥 과거의 코드를 답습하는 것이 아니라 어떤 선택지가 있고, 그 중에 어떤 방법을 왜 선택하는게 좋은지도 같이 고민해보신다면 하쿠의 성장에도 더 도움이 될거라고 생각합니다!
몇 가지 질문 추가로 남겼는데 같이 확인하고 넘어가면 좋을거 같아 Request changes를 드렸습니다!
고생 많으셨어요 하쿠 ~~ 👍 👍 👍
src/util/validation.ts
Outdated
| const isValidNumber = (number: string) => { | ||
| const regex = /^[0-9]*$/; | ||
| if (regex.test(number)) { | ||
| return true; | ||
| } return false; | ||
| } | ||
| if (Number.isInteger(number)) { | ||
| return true; | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거는 취향의 영역도 조금 있는데 아래처럼 작성해도 좋을거 같아요
const isValidNumber = (number: string) => {
return Number.isInteger(number)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오 그렇군요..! 알겠습니다!
src/util/validation.ts
Outdated
| const isValidLength = (number: string, maxLength: number) => { | ||
| if (number.length < maxLength) { | ||
| return false; | ||
| } return true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문] 왜 유효성 검사를 함수로 분리해서 모아놓는 것이 좋다고 판단하셨는지 궁금합니다!
| } | ||
|
|
||
|
|
||
| function App() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수 선언문과 화살표 함수의 차이를 찾아보시고 의도에 맞는 코드를 작성하실 수 있으면 좋을거 같네요 ㅎㅎ
src/util/validation.ts
Outdated
| } | ||
|
|
||
| const isValidNumber = (number: string) => { | ||
| const regex = /^[0-9]*$/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[질문] Number.isInteger() 를 사용했을 때 거르지 못 하는 케이스가 어떤게 있을까요?
src/util/validation.ts
Outdated
| const isValidMonthRange = (number: string) => { | ||
| if (Number(number)<=12 && Number(number)>=1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
입력할 때 number 타입으로 사용할 시 후처리해야할 것이 많아
[질문] 어떤 것들이 있었나요?
| const errorMessage = { | ||
| length: '2자리만 입력 가능합니다.', | ||
| number : '숫자만 입력 가능합니다.', | ||
| monthRange: '유효기간은 1~12월 사이여야 합니다.', | ||
| yearRange: '유효기간은 25~99년 사이여야 합니다.' | ||
| } | ||
| length: '2자리만 입력 가능합니다.', | ||
| number: '숫자만 입력 가능합니다.', | ||
| monthRange: '유효기간은 1~12월 사이여야 합니다.', | ||
| yearRange: '유효기간은 25~99년 사이여야 합니다.', | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수를 정의하는 하쿠의 컨벤션이 있나요? 상수가 정의 된 방식이 제각각인거 같네용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반 숫자의 경우 의미가 드러나보이는게 가독성에 좋다고 판단되면 상수화를 하고, 메시지들의 경우 여러곳에서 쓰이는 메시지는 따로 모아서 분리하고, 해당하는 곳에서만 쓰이면 그 파일에 두는 편입니다. 그러나 이번에는 number, length를 분리하려고 했을 때 유효기간의 경우 월, 년의 에러메시지가 같이 포함되어있어 number, length만 따로 분리해서 파일에 두는것보다 반복되더라도 사용하는 파일에 두는것으로 결정했습니다!
| placeholder="1234" | ||
| isError={isCardNumberInvalid(cardNumbers['fourth'])} | ||
| /> | ||
| {inputKeys.map((key) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yujo11
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 빠르게 확인해주셨군요 ㅎㅎ
이번 step은 여기서 merge하고 다음 step으로 넘어가도 좋을거 같네요!
고생 많으셨습니다 하쿠 다음 step에서 다시 만나요 ~~ 👍 👍 👍




안녕하세요! 유조🙂
이번 미션 리뷰어를 맡아주셔서 영광입니다.
🎯 미션 소개
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
1️⃣ 컴포넌트 구조
2️⃣ CSS 스타일
Styled-components vs Emotion vs CSS Module
Emotion은 검색을 통해 확인해본 결과, 다른 라이브러리에 비해 학습과 사용이 비교적 복잡해 보였습니다. 주어진 시간이 3일밖에 되지 않아 빠르게 적용하기에는 어려움이 있을 것 같아 제외하게 되었습니다.
CSS Module은 간단하게 사용할 수 있다는 장점이 있지만, props를 활용한 동적 스타일링에는 제약이 있습니다. 이번 프로젝트에서는 props에 따라 스타일이 동적으로 변경되어야 했기 때문에 적합하지 않다고 판단했습니다.
Styled-components는 props 기반의 동적 스타일링이 가능하고, JS 파일 안에 CSS를 함께 작성할 수 있어 파일 간 이동을 줄일 수 있다는 장점이 있습니다. 다만 JS와 CSS 코드가 모두 길어질 경우 한 파일 내에서 관리하는 것이 오히려 불편할 수 있다는 단점도 있습니다.
결론적으로, 동적 스타일링이 필요하고 사용법이 간단한 Styled-components를 선택하여 프로젝트에 적용하였습니다.
3️⃣ 스토리 파일을 component 폴더에 모은 이유
스토리 파일의 위치를 고민했을 때, ‘기능 조직’과 ‘목적 조직’ 중 목적 조직 형태를 택하고자 했습니다.
stories/ 폴더에 스토리 파일을 모으는 것은 기능 중심으로 구성하는 방식이라 판단했고, 반면 components/ 폴더 내부에 스토리를 함께 관리하는 방식은 목적 중심 조직이라 판단했습니다.
목적 조직을 선택한 이유는 구조 내 응집도를 높이고, 빠른 피드백 루프를 확보할 수 있기 때문입니다. 따라서 각 컴포넌트 폴더 안에 스토리 파일을 함께 관리하기로 결정했습니다.
4️⃣ 상수화
처음에는 key와 value가 동일한 경우 굳이 상수화할 필요가 있을까 고민했습니다.
하지만 문자열을 직접 사용하는 것보다 상수로 선언해두면 자동완성, 오타 방지, 유지보수 측면에서 장점이 있다고 판단하여 상수화를 진행했습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
1️⃣ CSS 스타일
✅ 리뷰어 체크 포인트
1. 컴포넌트 설계
2. 상태 관리
3. Storybook 활용
4. UI/UX