-
Notifications
You must be signed in to change notification settings - Fork 210
[1단계 - 페이먼츠] 쑤쑤(현수연) 미션 제출합니다. #364
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: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[email protected]>
Co-authored-by: soosoo22 <[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.
안녕하세요 쑤쑤
방학은 잘 지내셨나요 😆 센스 있는 2행시 감사합니다 🤣
styled-components가 설치된 package.json을 따로 커밋을 안 하신 것 같아요!
제 로컬에서 확인해봤을 때는 styled-components가 없어져 있더라고요 한 번 확인 부탁드려요 :)
저도 우테코 리액트 미션을 처음 시작할때 리액트에 대한 공부가 부족해서 같이 미션을 한 페어들에게 많은 도움을 받았었습니다 쑤쑤도 우테코 미션을 하나 둘 씩 해나가면서 리액트에 대해서 경험을 쌓아가시길 바래요 파이팅입니다 💪
src/components/PaymentApp.tsx
Outdated
const [cardNumbers, setCardNumbers] = useState<CardInfo[]>([]); | ||
const [expirationDate, setExpirationDate] = useState<CardInfo[]>([]); | ||
const [userName, setUserName] = useState<CardInfo[]>([]); | ||
|
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.
상태관리를 최상단에서 3개로 해주고 있어서 고민이라고 pr에 작성해주셨는데요
저는 정말 좋은 미션 접근 방식이라고 생각합니다 👍
이렇게 하나 씩 고민해 나가는 것이 좋다고 생각합니다
(추가로 저는 하나로 통합하는 것 보다 3가지로 분리해서 관리하는 것이 더 좋다고 생각합니다 왜냐하면 하나로 통합하면 카드 폼에 입력해야할 항목이 늘어날 수록 관리하기 힘들다고 생각해요)
추가로 리액트를 우테코에서 공부하는 방법을 하나 제안드릴까합니다
제가 생각했을 때 우테코 미션을 진행하가면서 좋은 접근 방식은
처음에 간단하게 요구사항을 만족시키도록 구현한다 => 2단계 요구사항과 미션 피드백을 반영하면서 불편함을 느끼고 이를 개선해나간다 라고 생각하고 있어요
그런 측면에서 쑤쑤의 지금 현재 단계에서 상태관리를 app에서 모두 해주는 건 좋은 접근 방식이라고 생각합니다
state로 app에서 모두 선언해서 관리하고 props로 각각 컴포넌트에 넘겨주기 -> 2단계 미션 요구사항 반영 및 리뷰 반영하면서 app에서 props로 넘기는 구현 방식이 불편함을 느끼고 -> 해당 불편함을 개선해 나가면서 리팩토링 해보기
단계로 공부를 진행해나가보시는 것도 좋을 것 같아요
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.
오! 감사합니다~🤩
아무래도 하나로 통합하지 않고 3가지로 분리하여 관리한 경우에 각각의 컴포넌트에 props를 전달하는 과정이 번거로웠던 것 같아요!!! 그래서 상태 관리를 통합해서 사용해야 할지 고민했습니당
오! 리액트 공부 꿀팁! 감사합니다!
직접 구현해보며 불편함을 느끼고 개선해 나가는 과정이 중요하다는 말씀이시군요!
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.
넵넵 불편함을 느끼고 개선해보는 경험이 저는 개인적으로 중요하다고 생각해요!
불편함을 느꼈을 때 특정 문법들 (=예를 들어 context api와 같은)이 왜 생겨났는지 이해하기 쉬웠기에
쑤쑤도 이런 경험을 해보시면 좋을 것 같아요
src/components/Card/CardPreview.tsx
Outdated
}) => { | ||
return ( | ||
<> | ||
<CardFrame> |
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.
개인적으로 styled component로 만든 스타일링을 위한 컴포넌트는 다른 컴포넌트들과 구별이 되어야 한다고 생각해요
왜냐하면, 스타일링을 고쳐야할 일이 있을 때 컴포넌트 간에 빠르게 구분하여 스타일을 수정해줄 수 있다는 장점 등이 있습니다
그래서 사용하실 때 <Styled.CardFrame>
처럼 Styled로 prefix를 붙여서 구분해주는 것도 좋다고 생각합니다
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.
아하! Styled로 prefix를 붙여서 구분하는 방법이 있군요!
나름 구분하기 위해 ExpirationDateStyled
와 같이 네이밍을 짓고 있었는데, 이제 prefix를 붙여서 구분하는 방식으로 변경하겠습니다^^ 감사합니당~😄
|
||
const CardNumber = ({ number }: { number: string }) => { | ||
return <CardNumberWrapper>{number}</CardNumberWrapper>; | ||
}; |
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를 children으로 받아서 값을 넣어줘도 좋을 것 같아요
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/components/Card/UserName.tsx
Outdated
{Object.keys(latestUserNames).map((index) => ( | ||
<UserNameStyled key={index}> {latestUserNames[Number(index)]}</UserNameStyled> | ||
))} |
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.
latestUserNames의 value들만 뽑아와서 map을 돌려도 되지 않을까요?
src/components/Form/Form.tsx
Outdated
<InputDescription | ||
title="결제할 카드 번호를 입력해 주세요." | ||
description="본인 명의의 카드만 결제 가능합니다." | ||
></InputDescription> |
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.
컴포넌트 사이에 children이 없다면 tag를 바로 닫아주는 것이 좋아요!
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/components/Form/Input.tsx
Outdated
<InputStyled | ||
isValidInput={isValidInput} | ||
onChange={inputChangeHandler} | ||
maxLength={maxLength} | ||
type={type} | ||
placeholder={placeholder} | ||
></InputStyled> |
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.
input의 value에 state값 value를 넣어 상태값의 최신 상태를 반영하도록 해주는 것이 좋아요
여기서는, setData로 넘겨주고 있는 PaymentApp에서 선언된 상태값들을 넣어줘야 겠네요
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.
아아 꽤나 중요한 부분을 놓치고 있었네요ㅠㅠ
원래는 PaymentApp에서 ‘CardNumberForm, ExpirationDateForm, UserNameForm’ 컴포넌트에 상태를 업데이트 해주는 함수만 넘겨주는 식으로 구현했었는데 state값이랑 함수 모두 전달하는 식으로 변경하는게 좋을 것 같네요. 그 후에 input의 state값을 넣어주면 될 것 같습니당!
감사함당~🤩
src/components/Form/Input.tsx
Outdated
validationRule, | ||
errorMessageText, | ||
}: InputProps) => { | ||
const [currentValue, setCurrentValue] = useState(""); |
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.
useInput내에서 useState를 한 번 더 선언해줄 필요가 있을까요?
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 inputs = Array.from({ length: inputCount }, (_, index) => ( | ||
<Input | ||
key={index} | ||
index={index.toString()} | ||
type={type} | ||
placeholder={placeholders[index]} | ||
maxLength={4} | ||
setErrorMessage={setErrorMessage} | ||
setData={setCardNumbers ? setCardNumbers : () => {}} | ||
setAllInputValid={(isValid) => updateInputValidity(index.toString(), isValid)} | ||
validationRule={(value) => /^[0-9]{4}$/.test(value)} | ||
/> | ||
)); | ||
|
||
return <FormElement labelContent={labelContent} inputs={inputs} errorMessage={errorMessage} />; |
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.
children속성을 활용해서 Input 배열을 넘겨주는 건 어떨까요?
const meta = { | ||
title: "PaymentApp", | ||
component: PaymentApp, | ||
} satisfies Meta<typeof PaymentApp>; | ||
|
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.
storybook은 컴포넌트 asset들을 표현한다고 생각해요
그렇기 때문에 app의 전체모습인 PaymentApp 같은 경우는 스토리북으로 보여주지 않아도 좋을 것 같아요
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 meta = { | ||
title: "UserName", | ||
component: UserName, | ||
} satisfies Meta<typeof UserName>; |
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.
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.
아하 white로 되어 있어서 보이지 않는 거였군요
…act-router-dom"과 "styled-components"를 추가
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.
안녕하세요! 샐리~ 쑤쑤예요!
일단 리뷰 요청을 너무 늦게 한 것 같아서 죄송해요ㅠㅠㅠ 최대한 빨리 리뷰 요청을 보내고 싶었지만 눈에 밟히는 것들이 하나하나씩 보이드라고요^^..
🏋️ 재배포
❗️변경사항
- 기존 CardInfo 타입을 string 배열로 변경하고 각각의 상태 변수를 문자열 배열로 초기화 했습니다. 카드 번호에는 4개의 빈 문자열이, 유효 기간에는 2개의 빈 문자열이, 사용자 이름에는 1개의 빈 문자열이 들어가도록 설정했습니다!
const [cardNumbers, setCardNumbers] = useState<string[]>(["", "", "", ""]);
const [expirationDate, setExpirationDate] = useState<string[]>(["", ""]);
const [userName, setUserName] = useState<string[]>([""]);
- 입력값의 상태를 최신 상태로 유지하기 위해 Form 컴포넌트에 cardNumbers, expirationDate, userName 및 해당 상태값을 업데이트하는 set 함수들을 props로 전달했습니다. 그런 다음 InputWrapper 컴포넌트에서는 해당 상태값을 value로 넣어줬습니다.
<Form
{...{ cardNumbers, setCardNumbers }}
{...{ expirationDate, setExpirationDate }}
{...{ userName, setUserName }}
/>
<Styled.InputWrapper
isValidInput={isValidInput}
onChange={inputChangeHandler}
maxLength={maxLength}
type={type}
placeholder={placeholder}
key={index}
value={data[index]}
/>
- 스타일 컴포넌트를 구분하기 위해 Styled를 prefix로 붙였습니다! 스타일 수정이 필요할 때 컴포넌트 끼리 구별이 되어서 빠르게 찾을 수 있다는 장점이 있는 것 같습니다!!
const Styled = {
InputLabel: styled.label`
font-size: 12px;
font-weight: 500;
`,
};
<Styled.InputLabel htmlFor={labelContent}>{labelContent}</Styled.InputLabel>
- 스토리북에서 보이지 않았던 컴포넌트(username, cardnumber, cardnumbers, expirationDate)들이 알고 보니 컬러가 화이트로 설정이 되어 있어서 보이지 않았던 거였습니다...😅 그래서 CardPreview 컴포넌트에 컬러를 화이트로 적용하고, 그 안에 있는 각 컴포넌트들의 화이트 설정을 제거했습니다!
😭 아쉬운 점
- 유효성 검사
유효 기간 입력란에서 월과 연도를 각각 입력하는데, 월은 01부터 12까지의 숫자를 입력해야 하고, 연도는 현재 연도부터 시작하는 연도여야 합니다. 각각의 조건이 맞지 않을 경우에는 해당하는 에러 메시지를 출력해야 하는데, 현재는 "만료 기한을 올바르게 입력해 주세요"라는 메시지만 보여주고 있어서 상황에 맞는 구체적인 에러 메시지가 아니어서 아쉬움이 남았습니다.
setErrorMessage(allValid ? "" : "만료 기한을 올바르게 입력해주세요.");
- 카드 번호가 카드 프리뷰 각 자리에 맞지 않음
예를 들어 세 번째 입력란에 숫자를 입력하면 CardPreview에 세 번째 자리에 보여져야 하는데 살짝 앞쪽에 보여지는 문제가 있습니다. 각 자리에 맞는 곳에 숫자가 보이지 않아서 애를 먹었습니다ㅠㅠ flex: 1
을 추가 했었는데 그럼에도 살짝 문제가 되는 부분들이 있어서 일단 이 부분은 고민해 보고 차차 수정해 보겠습니다😭
바쁘신데도 불구하고 정성스러운 피드백 감사합니다! 샐리!
2024년 돈 많이 버시고~💵💵💵 행복하세요~~🍀🍀🍀
const Styled = { | ||
CardLogoWrapper: styled.img` | ||
width: 36px; | ||
height: 22px; | ||
gap: 0px; | ||
opacity: 0px; | ||
`, | ||
}; |
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.
Styled로 구분해주셨군요 👍👍
추가로 Styled관련 컴포넌트 관리에 더 도움이 되시라고 추가 정보를 공유하지만
주로 styled component를 위치시키는 방법에는 크게 2가지 가 있습니다
- Styled관련 컴포넌트를 분리하여 import해오는 방식
예를 들어, CardLogo.styles.ts 처럼 파일 네이밍을 지어서 CardLogo.tsx 와는 분리해서 선언해주는 방식이 있어요
- CardLogo.tsx에서 스타일을 선언하는 방식
지금 쑤쑤가 한 것 처럼 한 파일에 같이 관리하는 경우도 있습니다
이건 개인의 선호도가 들어가 있기 때문에 개인적으로는 뭐가 더 낫다고 생각하지는 않고요
쑤쑤도 한 번 이것 저것 경험해보시면 좋을 것 같아서 공유드립니다 😊
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.
코드가 점점 길어지다 보니 Styled관련 컴포넌트를 분리 해야겠다는 생각을 했습니다. 차차 리팩을 진행하면서 적용해 보도록 하겠습니다!
<Styled.CardNumbersWrapper> | ||
<CardNumber number={cardNumbers[0]} /> | ||
<CardNumber number={cardNumbers[1]} /> | ||
<CardNumber number={Array(cardNumbers[2].length).fill(<Styled.Dot />)} /> | ||
<CardNumber number={Array(cardNumbers[3].length).fill(<Styled.Dot />)} /> | ||
</Styled.CardNumbersWrapper> |
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.
카드 번호가 카드 프리뷰 각 자리에 맞지 않아서 아쉽다고 남겨주셨네요
제 생각에는 그런 경우라면, 프리뷰에서 numbers들의 위치를 고정해두어야 한다고 생각해요!
CardNumbers를 이루고 있는 각각의 card number에 min-width 같은 속성을 활용해서 최소 width를 확보해두어서
동적으로 위치가 변하지 않도록 하거나 width를 고정 해두어서 각각 정해진 크기만큼 차지하게 하도록 하는 것이 좋아보여요
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.
아하! min-width 를 주는 방법이 있었네요! 감사합니다!
const inputs = Array.from({ length: inputCount }, (_, index) => ( | ||
<Input | ||
key={index} | ||
index={index.toString()} | ||
index={index} | ||
type={type} | ||
placeholder={placeholders[index]} | ||
maxLength={4} | ||
setErrorMessage={setErrorMessage} | ||
setData={setCardNumbers ? setCardNumbers : () => {}} | ||
setAllInputValid={(isValid) => updateInputValidity(index.toString(), isValid)} | ||
data={cardNumbers || []} | ||
setData={setCardNumbers || (() => [])} | ||
setErrorMessage={(errorMessage) => setErrorMessage(errorMessage)} | ||
setAllInputValid={(isValid) => updateInputValidity(index, isValid)} | ||
validationRule={(value) => /^[0-9]{4}$/.test(value)} | ||
/> |
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.
children 속성을 잘 활용하면 재사용성도 높이고 코드의 가독성도 높일 수 있다고 생각해요!
예를 들어, FormElement에 children속성을 활용해서 input 배열을 넣어주게 되면 FormElement 안에는 input들이 있겠구나 한 번에 파악 할 수 있기 때문이에요
같은 맥락으로 이곳도 아래처럼 대체가 가능하다고 생각해요!
<FormElement>
{
Array.from({ length: inputCount }, (_, index) => <Input />
}
</FormElement>
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.
훨씬 가독성이 높아지네요! children 속성을 잘 활용해 보도록 하겠습니다!
const meta = { | ||
title: "UserName", | ||
component: UserName, | ||
} satisfies Meta<typeof UserName>; |
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.
아하 white로 되어 있어서 보이지 않는 거였군요
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.
안녕하세요 쑤쑤
어제 저녁에 리뷰 요청을 보내주셨는데 어제 오늘 바빠서 지금 리뷰를 하게 되었네요 😅
오늘 하루는 잘 보내셨나요?
몇 가지 더 반영해보면 좋을 것 같은 점들을 코멘트로 남겼어요
한 번 읽고 2단계 개발 시작 전에 반영하고 진행하셔도 좋을 것 같아요
2단계도 파이팅입니다 💪
안녕하세요 샐리~~ 쑤쑤예요!
영화 리뷰 미션에 이어 페이먼츠 미션에서도 만나네요! 너무 반가워요~😉 (운명...?? 아닐까요?)
저번 미션에서 추천해주신 밤양갱 아주 잘~ 들었습니다^^~
일단 미리 알려 드려야 할 것 같아서 전달드립니다. 타입스크립트를 적용하는 과정에서 생겨난 몇몇 타입 에러들이 있습니다. 이들을 해결하지 못한 채 배포를 진행했습니다. 에러가 존재하는 코드 맨 위에
// @ts-nocheck
추가해서 빌드 후 배포했습니다. 이러면 안된다는 걸 알지만😭.. 일단 배포는 해야 했기 때문에 선택했습니다. 참고하시고 봐주시면 감사하겠습니다! 에러는 꼭! 해결해 보겠습니다!😎🏋️ 배포 주소
배포 링크
스토리북 링크
💡 구조
styled-components, 스토리북 사용
❓ 질문
1. 상태관리
최상위 컴포넌트(PaymentApp)에서 useState로 상태관리를 했습니다. 여기서 카드 번호, 유효 기간, 사용자 이름 총 3개의 useState를 사용했는데 하나로 합쳐서 통합해서 관리를 해야 하는지 의문점이 들었습니다. 샐리는 어떻게 생각하시나요
😩 아쉬운 점
1. 리액트 이해도 부족
저번 타입스크립트 때 처럼 리액트에 대한 이해도가 부족해서 미션을 진행하면서 이런 식으로 하는 게 맞는건가? 의문점이 많이 들었습니다. 시간이 촉박해서 일단 돌아가는 쓰레기를 구현하긴 했으나 만족하는 코드는 아니였어서 리팩하며 차차 고민해보며 기초를 다져야 할 것 같습니다.👍
2. 타입 에러를 해결하지 못 한 부분
리액트가 익숙치 않다 보니 타입 에러까지 신경 쓰지 못 한 것 같습니다. 그렇다 보니 에러가 하나하나씩 쌓이며 배포 때 문제가 생겼습니다.. 뒤늦게 에러를 해결 할려고 했지만 시간이 촉박했고 해결되지 않는 에러들이 있어서 일단 임시방편으로 에러를 무시하는 코드를 넣고 배포를 진행했습니다. 어떻게 보면 꼼수니까.. 다음부턴 예의 주시하며 진행하도록 하겠습니다~😭
2. 스토리북 환멸
이번 미션을 통해서 처음으로 스토리북을 사용해 봤는데 반감만 더 커졌습니다^^ 솔직히 상태관리도 힘든데 스토리북도 신경을 써야 했고 이로 인해 일을 2배로 늘리게만 했던 짐같은 존재였습니다… 아직 제대로 사용하지 않아서 유용함을 못 느낀 것 같긴 합니다.. 다음 단계에선 보다 유용함을 느끼며 사용하길 바래봅니다.🥹
긴 글 읽어주셔서 감사합니다! 샐리로 2행시를 하며 마무리할게용~
샐: 샐리~ 저번에 선릉캠퍼스에 왔었다는 소식을 들었어요!
리: 이번엔 안 오시나요?? 기다릴게용~🤩🤩