[클린코드 리액트 3기 조용준] 페이먼츠 미션 Step1 #118
Conversation
- `GlobalStyles.tsx` 전역 스타일을 설정 - `styleToken.ts` 색상, 폰트 크기, 폰트 두께 등의 스타일 토큰을 포함하는 객체 추가 - `DefaultStyled.tsx` 기본 스타일 속성이 적용된 스타일 컴포넌트를 반영 - `storybookControls.ts` 컴포넌트의 Props에 대한 Storybook 컨트롤 옵션을 정의
- preview에서 action과 control의 parameter 추가
- main branch push, 특정 디렉토리에 변경사항이 있을 때 실행
- `Box` 컴포넌트 상위 계층을 묶어주는 레이아웃 - `VStack` flex column 레이아웃 - `VStack` flex row 레이아웃
There was a problem hiding this comment.
안녕하세요 용준님!
페이먼츠 미션을 함께하게 된 리뷰어 황준일입니다 🙇♂️
코드를 무척 체계적으로 작성해주셨네요 ㅎㅎ
대체로 읽기가 수월했던 것 같아요.
정리도 잘 해주셔서 좋았습니다!
Controlled Component 구현 시, 구성 요소(props)를 모두 외부에서 받는 것이 좋을지 대해 궁금합니다.
말씀해주신 것 처럼 Compound Component 로 만드는 방법도 있을 것 같아요.
이럴 때 고민해볼 부분이 지금은 CardInput인데 (Card에 종속적인), 만약 중간 레이어의 컴포넌트가 생겨서 이걸 다른 도메인에서도 사용해야 한다고 했을 때를 고민해보면 좋을 것 같아요.
가령, 아예 CardInput으로 확정짓는다면 Compound Component로 도메인 종속적인 모습으로 만들어도 좋지만
이게 UI로 표현이 되어야 한다면, props를 통해서 조합하는 방식을 고민해봐야 할 것 같아요.
"Card라는 단어를 제거하고 이 UI와 기능을 표현한다면 어떻게 해야 좋을까?" 에 대해서 고민해보는거죠 ㅎㅎ
Card Form 안에서의 focus 전환 방식에 궁금합니다.
위의 내용과 동일합니다! focus 전환에 대한 조건을 실행하는 함수를 넘겨줄 수도 있겠죠?
정답은 없다고 생각해요. 다만 내가 이 기능에 대한 도메인을 확정지을지 혹은 확장가능성을 남겨놓을지에 따라 구현 방향성이 달라진다고 생각합니다.
그리고 전체적인 피드백의 내용을 정리해보자면,
components에 도메인과 관련된 내용이 들어있는 것 같아요!
그래서 지금 구조에 하나의 계층이 더 추가되면 어떨까 싶습니다!
함수 네이밍을 할 때 handle을 무척 자주 사용하시는 것 같아요.
handle -> 이벤트 핸들러인데, 이벤트 핸들러와 함수는 어떤 차이가 있는걸까요?
컴포넌트가 외부에서 어떤 동작을 주입 받을 때 이벤트를 통해서 주입 받는건데 그렇기 때문에 이벤트에 대한 정의가 무척 중요하다고 생각해요.
그래서
- 이벤트는 뭘까?
- 이벤트 핸들러는 뭘까?
- 함수는 뭘까?
위에 대해 조금 더 깊게 생각해보면 좋지 않을까 싶습니다!
어플리케이션에 약간의 버그가 있는 것 같아요 ㅎㅎ
한 번 확인 부탁드립니다 🙏
이번에 준일님이 진행해주신 클린코드 세션에서 많이 영감을 받았어요.
CardInput 요소들을 props를 전달받도록 개선했어요.
컴포넌트 확장에 대한 내용을 조금 더 고민해보겠습니다!
습관적으로 handled prefix를 붙인 것 같아요.
컴포넌트 인터페이스를 정의하면서도 명확하게 구성해볼수 있도록 해야겠다는 점들을 알아가게 됐어요. 페이먼츠 첫 리뷰 감사합니다! 준일님 리뷰 주신 코멘트를 보면서, 피드백 주신 내용 중에 리뷰 요청 시간이 많이 늦어질 것 같아 먼저 반영된 부분들만 요청드려봅니다. 🙇♂️ |
|
리뷰 요청 드리고, 늦었지만 리뷰 주신 내용 모두 반영했어요! (Input 공통 컴포넌트로 개선 제외 😥) |
|
감사합니다! 저도 오늘 중으로 피드백 드리도록 하겠습니다 😄 |
| paths: | ||
| - 'src/components/**' | ||
| - 'src/shared/components/**' | ||
| - 'src/shared/styles/**' |
| steps: | ||
| - uses: actions/checkout@v2 | ||
| with: | ||
| fetch-depth: 0 # 전체 Git 이력을 가져오도록 설정 |
There was a problem hiding this comment.
지금은 전체 이력을 가져올 필요가 있을까 싶어요!
다른 브랜치를 사용하는 것도 아니고 푸시나 풀을 하는 것도 아니라서요!
There was a problem hiding this comment.
지금은 전체 이력을 가져올 필요가 있을까 싶어요! 다른 브랜치를 사용하는 것도 아니고 푸시나 풀을 하는 것도 아니라서요!
제안주신 것처럼 설정에서 제외해도 되겠어요! 👀
기본 설정으로도 문제 없어보여요.
| @@ -0,0 +1 @@ | |||
| v20.11.1 | |||
There was a problem hiding this comment.
오호.. nvmrc 가 있는 것도 처음 알았네요 ㅎㅎ
회사에서 프로젝트마다 node version이 달라서 잘 활용하고 있어요 🙂
| alias: { | ||
| '@': path.resolve('./src'), | ||
| }, |
There was a problem hiding this comment.
아마 나중에 패키지를 배포할 일이 있을 때는 이렇게 alias를 사용하게 되면 문제가 될 수 있답니다!
-> 어떤 문제가 있을 수 있을까요?
There was a problem hiding this comment.
아마 나중에 패키지를 배포할 일이 있을 때는 이렇게 alias를 사용하게 되면 문제가 될 수 있답니다! -> 어떤 문제가 있을 수 있을까요?
음.. 지금 개발 중인 컴포넌트들을 패키지로 배포한다면,
지금 환경은 bunlder에서 alias를 설정했기에 파일경로를 인식해주고 있어요.
패키지를 활용하는 프로젝트에서는 모듈을 인식하지 못할수 있겠다? 는 생각이 들었습니다.
| export type CardState = { | ||
| cardNumber: string[]; | ||
| expirationDate: string[]; | ||
| ownerName: string; | ||
| securityCode: string; | ||
| label: string; | ||
| color: string; | ||
| description: string; | ||
| }; |
There was a problem hiding this comment.
| export type CardState = { | |
| cardNumber: string[]; | |
| expirationDate: string[]; | |
| ownerName: string; | |
| securityCode: string; | |
| label: string; | |
| color: string; | |
| description: string; | |
| }; | |
| type CardNumber = `${number}${number}${number}${number}`; | |
| type ExpirationMonth = `${0}${number}` | `1${0 | 1 | 2}` | |
| type ExpirationYear = `${number}${number}` | |
| export type CardState = { | |
| cardNumber: [CardNumber, CardNumber, CardNumber, CardNumber]; | |
| expirationDate: [ExpirationMonth, ExpirationYear]; | |
| ownerName: string; | |
| securityCode: `${numbe}${number}${number}`; | |
| label: string; | |
| color: string; | |
| description: string; | |
| }; |
이런식으로 타입을 더 정교하게 작성할 수 있답니다!
There was a problem hiding this comment.
이런식으로 타입을 더 정교하게 작성할 수 있답니다!
😲 Array tuple 타입을 알고 있었는데, 저렇게 string type도 명시적으로 쓸 수 있는지 몰랐어요! 👍👍👍
| type SecurityCodeInputProps = Partial<{ | ||
| refs: RefObject<HTMLInputElement>; | ||
| value: string; | ||
| onChange: (event: ChangeEvent<HTMLInputElement>) => void; | ||
| onKeyUp: (event: KeyboardEvent<HTMLInputElement>) => void; | ||
| }>; | ||
|
|
||
| export const CardSecurityCodeInput = forwardRef<HTMLInputElement, SecurityCodeInputProps>( | ||
| ({ value, onChange, onKeyUp }, ref) => ( |
There was a problem hiding this comment.
refs는 쓰이고 있지 않네요!?
막바지에 급하게 반영하다 보니 놓친 부분들이 있네요 😭
앞서 리뷰 주신 내용과 같이 개선해 나가겠습니다.
| export const CardProvider = ({ children }: PropsWithChildren) => { | ||
| const [card, setCard] = useState<CardState>(INITIAL_CARD_STATE); | ||
|
|
||
| const [ownerCards, setOwnerCards] = useState<CardState[]>([]); | ||
|
|
||
| const resetCurrentCard = () => { | ||
| setCard(INITIAL_CARD_STATE); | ||
| }; | ||
|
|
||
| const addCardToOwner = (card: CardState) => { | ||
| setOwnerCards((prevCards) => [...prevCards, card]); | ||
| }; | ||
|
|
||
| const contextValue = useMemo( | ||
| () => ({ | ||
| card, | ||
| ownerCards, | ||
| setCard, | ||
| resetCurrentCard, | ||
| addCardToOwner, | ||
| }), | ||
| [card, ownerCards], | ||
| ); | ||
|
|
||
| return <CardContext.Provider value={contextValue}>{children}</CardContext.Provider>; | ||
| }; | ||
|
|
||
| export const useCard = () => { | ||
| const context = useContext(CardContext); | ||
| if (context === null) { | ||
| throw new Error('useCard는 CardProvider 하위에서 사용되어야 합니다.'); | ||
| } | ||
| return context; | ||
| }; |
There was a problem hiding this comment.
component의 역할을 하는게 아니다보니 아예 provider 라는 폴더를 하나 만들어서 관리해주면 어떨까 싶어요!
There was a problem hiding this comment.
component의 역할을 하는게 아니다보니 아예 provider 라는 폴더를 하나 만들어서 관리해주면 어떨까 싶어요!
네, providers 폴더를 하나 더 구성해서 분리해볼게요! 🙂
| export const CardSelectBottomSheet = ({ onOverlayClick, onSubmit }: CardSelectBottomSheetProps) => ( | ||
| <BackDrop onClick={onOverlayClick}> | ||
| <VStack | ||
| position="absolute" | ||
| bottom="0" | ||
| left="0" | ||
| width="100%" | ||
| height="230px" | ||
| backgroundColor="white" | ||
| borderRadius="5px 5px 15px 15px" | ||
| > | ||
| <Grid | ||
| gridTemplateColumns="repeat(4, 1fr)" | ||
| alignItems="center" | ||
| justifyContent="center" | ||
| height="100%" | ||
| padding="20px 20px" | ||
| > | ||
| {CARD_BRANDS.map(({ label, color }) => ( | ||
| <CardSelectButton | ||
| key={`card-select-${label}`} | ||
| color={color} | ||
| label={label} | ||
| onClick={() => { | ||
| onSubmit?.({ label, color }); | ||
| }} | ||
| /> | ||
| ))} | ||
| </Grid> | ||
| </VStack> | ||
| </BackDrop> | ||
| ); |
There was a problem hiding this comment.
submit 이라는 표현이 좋을까? 싶네요.
ui에서도 submit 이라는 행위를 하는건 아닌 것 같아요.
onClick이 더 낫지 않을까요!?
There was a problem hiding this comment.
submit 이라는 표현이 좋을까? 싶네요. ui에서도 submit 이라는 행위를 하는건 아닌 것 같아요. onClick이 더 낫지 않을까요!?
form을 submit 하는 역할을 하는건 아니네요!
onClick으로 표현하는게 더 직관적일 수 있겠어요.
CardSelectBottomSheet 컴포넌트의 경우에 클릭 요소가 Overlay와 Button이 존재하는데요.
이런 경우에 @JunilHwang 님은 props로 어떻게 전달하실까요?
onOverlayClick은 명확한 네이밍을 가지는데, 컴포넌트의 주요 클릭요소라고 판단된다면 onClick으로 전달해도 될지? 가 고민입니다. 컴포넌트에 여러 클릭요소가 있을때, 어떤 방식을 네이밍 하시는지 궁금합니다!
위 같은 경우,
CardSelectButton에 전달한 prop은 onCardSelectClick 구체적으로 지으면 좋을지? 궁금증이 남아있어 여쭤봐요.
There was a problem hiding this comment.
말씀해주신 것 처럼 onCardSelectClick 으로 표현하는 것 같아요 ㅎㅎ
아니면 아예 컴포넌트를 바깥에서 chidlren으로 넘겨줄 수 있도록 변경하거나?
| export const Primary: Story = { | ||
| render: () => ( | ||
| <Funnel.Root startIndex={1}> | ||
| <Funnel.Step index={0}> | ||
| <CardListPage /> | ||
| </Funnel.Step> | ||
| <Funnel.Step index={1}> | ||
| <CardAddPage /> | ||
| </Funnel.Step> | ||
| <Funnel.Step index={2}> | ||
| <CardCompletePage /> | ||
| </Funnel.Step> | ||
| </Funnel.Root> | ||
| ), | ||
| }; | ||
|
|
||
| export const WithStartIndex: Story = { | ||
| render: () => ( | ||
| <Funnel.Root startIndex={0}> | ||
| <Funnel.Step index={0}> | ||
| <CardListPage /> | ||
| </Funnel.Step> | ||
| <Funnel.Step index={1}> | ||
| <CardAddPage /> | ||
| </Funnel.Step> | ||
| <Funnel.Step index={2}> | ||
| <CardCompletePage /> | ||
| </Funnel.Step> | ||
| </Funnel.Root> | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Funnel.Step 에서 고민이 되는 부분이 어차피 자기 자신의 위치가 index인데 index를 props로 넘겨줄필요가 있을까요?
There was a problem hiding this comment.
Funnel.Step에서 고민이 되는 부분이 어차피 자기 자신의 위치가 index인데 index를 props로 넘겨줄필요가 있을까요?
지금의 구조는 순차적인 한 방향 구조를 가지지만, 여러 조건에 의해서 Funnel의 Step을 넘나들어야한다면
외부에서 구성하는 Display index를 정의하고 이동시키면 어떨까 하는 생각이었어요.
예를들어
- List -> Cart
- List -> Add
같이 이동 될 수 있는 요구사항이 있고, 더 많은 페이지 고려가 있다면
전환 될 Display index 값이 몇번째인지 컴포넌트 구조를 보면서 확인해야하지 않을까?라는 생각이 들었어요.
enum CardPageStep {
List,
Cart,
Add,
Success,
}move(CardPageStep.Cart) 활용한다면 간단하겠다 생각되어 props로 전달하게 됐어요.
src/main.tsx
Outdated
| import { CardProvider } from '@/components'; | ||
| import { GlobalStyles } from '@/shared/styles'; | ||
|
|
||
| ReactDOM.createRoot(document.getElementById('root')!).render( | ||
| <React.StrictMode> | ||
| <GlobalStyles /> | ||
| <CardProvider> | ||
| <App /> | ||
| </CardProvider> | ||
| </React.StrictMode>, | ||
| ); |
There was a problem hiding this comment.
맞습니다! 혹시 components에 대해 어떻게 정의하고 계신가요?
|
#118 (comment)
src/components는 시각적 요소, 사용자 인터렉션을 담당하는 UI를 정의했습니다. |
안녕하세요, @JunilHwang 님 :)
아직 날씨가 많이 쌀쌀하네요. 밤낮으로 일교차가 심하니 감기 조심하세요!
페이먼츠 1단계 미션 피드백 부탁드립니다.
배포링크
1.mp4
필수 요구사항
카드 추가
기능 구현
카드리스트 페이지에서 카드 추가 버튼을 클릭하면 카드 추가 페이지로 이동합니다.카드추가 페이지에서 브랜드 카드를 선택합니다.카드등록완료 페이지로 이동합니다.카드등록완료 페이지에서 설명을 작성하고, 카드 등록 버튼을 클릭하면 카드가 등록됩니다.카드리스트 페이지로 이동합니다.구현 내용
Storybook
Primitive UI
src/shared/components에서 Primitive UI Component 를 구성했습니다.Chakra Component형식을 구현해보려고 시도했어요.DefaultStyled.tsx비교연산이 많아이 방식이 맞을까?의구심이 들었습니다.Domain Component
src/components에서 페이먼츠 도메인 컴포넌트를 구성했습니다.dot notation방식으로 구성했어요.Controlled Components 방식 구현
CardInput컴포넌트에 onChange Handler를 통해 입력값을 받아오는 방식으로 구현했습니다.uesRef를 사용해서focus를 주는 방식으로 구현했는데,focus를 주는 방식이 아쉬웠습니다.focus이동이 자유롭지 못한 부분이 아쉬웠어요.궁금한 점
TextField는 입력 완료 또는 Backspace 키 입력 시focus전환되고, 이를 위해 각TextField에 ref를 할당했어요.TextField를 Compound Component 방식으로 분리하여, 각각을 사용하는 곳에서 필요한 props를 전달하고 Context를 관리하면, 지금과 같이 하나의 컴포넌트에 4개의 ref를 전달하는 방식을 지양할 수 있다고 생각됐어요. 이 부분에서 준일님 의견이 궁금합니다!focus전환 방식에 궁금합니다.내부 요소들을 잘 제어하기 위해서는 어떠한 방식을 활용해보면 좋을까 하는 궁금증 입니다!