-
Notifications
You must be signed in to change notification settings - Fork 69
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
[클린코드 리액트 3기 이정수] 페이먼츠 미션 Step1 #150
base: dolilu
Are you sure you want to change the base?
Conversation
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.
안녕하세요 정수님! 일단 빠르게 1차 리뷰드릴게요.
원시적인 형태의 Primitive UI
형태의 컴포넌트 작성
구현하신 NumberInput
처럼 도메인 의존성이 없는 원자적 레벨의 컴포넌트로 UI를 구현하는 리팩토링을 시도해보세요. 더 자세한 내용은 nextstep의 UI Component를 참고해주세요.
RCA
각 리뷰 앞에 r, c, a로 리뷰의 우선순위를 나타냈으니 참고 부탁드립니다.
- r: Request Change. 꼭 반영해주세요.
- c: Comment. 참고해주세요.
- a: Approve. 바로 머지 가능한 코드입니다.
src/components/App.tsx
Outdated
const handleCardClick = () => { | ||
setIsModalOpen(true) | ||
} | ||
|
||
const handleCardNameChange = (value: string) => { | ||
setCardName(value) | ||
setIsModalOpen(false) | ||
} | ||
const handleInputChange = (value: string, field: string, setState: Dispatch<SetStateAction<any>>) => { | ||
setState(prevState => ({ | ||
...prevState, | ||
[field]: value | ||
})) | ||
} | ||
|
||
const handleExpiredDateChange = (value: string, field: string) => { | ||
handleInputChange(value, field, setExpireDate) | ||
} | ||
|
||
const handleCardNumberChange = (value: string, field: string) => { | ||
handleInputChange(value, field, setCardNumber) | ||
} |
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.
a: handle + 컴포넌트 + 이벤트 순으로 잘 작성해주셨네요. 패턴이 명확해 바로 이해할 수 있었습니다
src/components/App.tsx
Outdated
|
||
return ( | ||
<CardProvider> | ||
<> |
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.
c: 이미 <CardProvider>
로 감싸져 있어 굳이 fragment로 감쌀 필요는 없어 보입니다. fragment로 감싸게 되면 indent가 깊어지기 떄문에, 가독성을 위한 구분 목적으로는 공백 라인이 더 좋을 것 같다는 개인적인 생각입니다.
src/components/App.tsx
Outdated
<Card cardName={cardName} cardNumber={cardNumber} name={name} cardExpireDate={expireDate} | ||
onClick={handleCardClick}></Card> |
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.
a: 이렇게 prop이 많은 경우에는 prop마다 줄바꿈으로 구분하는 것이 가독성이 좋더라구요. 코드 스타일 관련이라 그냥 넘어가셔도 좋습니다.
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/App.tsx
Outdated
<Card cardName={cardName} cardNumber={cardNumber} name={name} cardExpireDate={expireDate} | ||
onClick={handleCardClick}></Card> | ||
<div className="input-container"> | ||
<span className="input-title">카드 번호</span> |
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.
r: NumberInput 컴포넌트로 숫자입력 잘 분리해주셨네요. 나머지 컴포넌트들도 미션 요구사항인 atomic한 컴포넌트를 구현해 bottom-up 방식으로 UI를 구성하는 방식으로 변경해주시기 바랍니다!
<span className="input-title">카드 번호</span> | |
<Label>카드 번호</Label> |
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.
리뷰가 늦어 죄송합니다!
Stepper 역할을 하는 Component를 직접 구현하는 미션인건지, 아니면 Stepper 검색을 하면 Material-UI Stepper가 많이 나오는 것 같은데 관련 라이브러리를 사용하는 부분인지 궁금합니다.
리액트 상태를 이용해 단계가 존재하는 UI의 단계 조작 로직을 쉽게 할 수 있도록 훅을 구현하시면 됩니다. Context API를 이용해 해당 요구사항 너무 잘 구현해주셨어요 💯
src/components/HeaderButton.tsx
Outdated
const HeaderButton: React.FC<HeaderButtonProps> = ({tag, className, step, value}) => { | ||
const {setCurrentStep} = useStepper() | ||
const handleClick = () => { | ||
if (step) { | ||
setCurrentStep(step) | ||
} | ||
} | ||
|
||
const Tag = tag as keyof IntrinsicElements; | ||
|
||
return ( | ||
<> | ||
<Tag | ||
className={className} | ||
onClick={handleClick} | ||
> | ||
{value} | ||
</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.
r: step을 prop으로 전달받고, 버튼 컴포넌트 내부에 해당 step으로 stepper 상태를 변경하는 로직을 선언하셨네요. 이렇게 구현하면 요구사항에 따라 유연하게 대처하지 못할 가능성이 많습니다. 컴포넌트를 유연하게 만들고 재사용하기 쉽게 만들기 위해서는 내부가 아닌 외부에서 클릭 이벤트에 대한 로직을 주입받도록 구현해, 외부에서 컴포넌트의 동작을 컨트롤하게 해주세요. 하나의 컴포넌트는 최대한 하나의 역할만 하도록 하는 것이 좋습니다.
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.tsx
Outdated
} | ||
const thirdSecret = third.length === 4 ? "****-" : "*".repeat(third.length) | ||
const fourthSecret = fourth.length === 4 ? "****" : "*".repeat(fourth.length) | ||
return `${first} ${second} ${thirdSecret} ${fourthSecret}` |
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.
c: 다음처럼 하이픈 다음 공백이 추가되어 렌더링 되는 걸 의도하셨나요?
1234- 1231- ****- ****
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/Display.tsx
Outdated
const Display: React.FC<DisplayProps> = ({className, value, defaultValue}) => { | ||
return ( | ||
<span className={className}>{value !== undefined && value !== "" ? value : defaultValue}</span> | ||
) | ||
} |
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.
a: placeholder 텍스트를 보여주는 컴포넌트군요. 👍 다음처럼 개선할 수 있을 것 같아요.
const Display: React.FC<DisplayProps> = ({className, value, defaultValue}) => { | |
return ( | |
<span className={className}>{value !== undefined && value !== "" ? value : defaultValue}</span> | |
) | |
} | |
const Display: React.FC<DisplayProps> = ({className, value, defaultValue}) => { | |
return ( | |
<span className={className}>{value || defaultValue}</span> | |
) | |
} |
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/Input.tsx
Outdated
const handleChange = (event: ChangeEvent<HTMLInputElement>) => { | ||
let value = event.target.value | ||
setInputValue(value) | ||
if (inputChange) { | ||
inputChange(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.
r:
- 스코프 내부에서
value
는 변경되지 않는 변수이기 때문에 const로 선언하는 것이 좀더 안전합니다. - inputChange처럼 옵셔널 콜백 인자는 옵셔널 체이닝을 통해 간결하게 호출할 수 있어요.
const handleChange = (event: ChangeEvent<HTMLInputElement>) => { | |
let value = event.target.value | |
setInputValue(value) | |
if (inputChange) { | |
inputChange(value) | |
} | |
} | |
const handleChange = (event: ChangeEvent<HTMLInputElement>) => { | |
const value = event.target.value | |
setInputValue(value) | |
inputChange?.(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.
수정했습니다.
src/interface/InputProps.ts
Outdated
export interface InputProps { | ||
type: string | ||
className: string | ||
maxLength?: number | ||
placeHolder?: string | ||
inputRule?: (value: string) => boolean | ||
inputChange?: (value: string) => void | ||
disabled?: boolean | ||
defaultState?: 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.
c:
- 리액트에서 export해주는 타입들을 사용하셔도 좋습니다.
inputChange
대신 change 이벤트 핸들러 콜백 인자명으로 익숙한onChange
그대로 사용하는 건 어떨까요?
export interface InputProps { | |
type: string | |
className: string | |
maxLength?: number | |
placeHolder?: string | |
inputRule?: (value: string) => boolean | |
inputChange?: (value: string) => void | |
disabled?: boolean | |
defaultState?: string | |
} | |
export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> { | |
inputRule?: (value: string) => boolean | |
} |
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/TextButton.tsx
Outdated
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.
r: 개발 중 만들어진 빈 파일은 삭제해주세요.
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 modifyCard = (newCard: CardType) => { | ||
setCards(prevCards => { | ||
const cards = prevCards.slice(0, prevCards.length -1); | ||
return [...cards, newCard]; | ||
}) | ||
} |
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.
c: 지금은 잘 동작하지만 추후 선택한 카드의 정보를 변경하는 경우 로직이 많이 변경되어 다른 컴포넌트에 수정이 전파될 것 같네요.
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.
마지막 요소만 수정가능한 것 같아 특정 카드를 선택해 정보를 수정하는 경우 id를 통한 비교가 이루어져야 할 것 같다는 의미였습니다. 지금은 잘 동작하니 넘어가셔도 좋습니다.
cardName: string | ||
cardNumber: { first: string, second: string, third: string, fourth: string } | ||
name: string | ||
cardExpireDate: string | ||
cardAlias: 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.
c: 비슷한 타입이 중복 선언되고 있네요. 공통화 가능할까요?
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/context/StepperContext.tsx
Outdated
export const StepperProvider: React.FC<{ children: React.ReactNode}> = ({ children }) => { | ||
const [step, setStep] = useState<string>("/") | ||
|
||
const setCurrentStep = (step: string) => { | ||
setStep(step) | ||
} | ||
|
||
return ( | ||
<StepperContext.Provider value={{ step, setCurrentStep }}> | ||
{children} | ||
</StepperContext.Provider> | ||
) | ||
} |
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.
a: Context API 사용해 stepper 잘 구현해주셨네요! 💯
src/form/AddCardCompleteForm.tsx
Outdated
function AddCardCompleteForm() { | ||
const {cards} = useContext(CardContext) | ||
const [cardAlias, setCardAlias] = useState('') | ||
const card = cards[cards.length-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.
r: 배열이 비어있을 경우 에러가 발생하는 코드입니다! 보다 안전한 방법으로 가져올 수 있도록 예외 처리해주세요
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.
a: 옵셔널체이닝 혹은 trycatch로도 가능한 점 참고해주세요
const card = cards[cards.length-1] | |
const card = cards?.[cards.length-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.
안녕하세요 정수님 피드백 반영 잘 해주셨어요 👍
이번에 리뷰 달지 않은 부분에서 이전에 남긴 리뷰가 적용되어야 하는 코드가 있었는데요, 이전에 이미 남겼기 때문에 중복해서 남기지는 않았습니다. (이벤트 핸들러 로직 상위에서 주입, 옵셔널 체이닝 호출 등)
import IntrinsicElements = React.JSX.IntrinsicElements; | ||
|
||
export interface HeaderButtonProps { | ||
tag: 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.
a: polymorphic하게 컴포넌트 구현해주셨네요 💯 정수님이 구현하신 것처럼 아토믹 컴포넌트에서 tag를 polymorphic하게 전달받으면 스타일은 재사용하면서 다른 html 요소의 기능을 사용할 수 있어 확장성이 개선됩니다.
src/form/AddCardForm.tsx
Outdated
<> | ||
<div className="app"> | ||
<HeaderButton className={"page-title"} tag={"h2"} value={"< 카드 추가"} step={"CardList"} event={() => handleClick("CardList")}/> | ||
<Card cardName={cardName} cardNumber={cardNumber} name={name} cardExpireDate={expireDate} |
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.
a: 결제와 같이 유사한 용어가 많은 도메인 개발 시 명확히 구분되는 네이밍을 고려해주세요.
- 카드사명
- 소유자명
- 카드 별칭
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/AddCard.tsx
Outdated
const AddCard: React.FC<AddCardProps> = ({cardName, cardNumber, name, expireDate}) => { | ||
const {addCard} = useContext(CardContext) | ||
const {setCurrentStep} = useStepper() | ||
|
||
const handleClick = () => { | ||
const {month, year} = expireDate | ||
const cardExpireDate = `${month} / ${year}` | ||
const newCard: CardType = {cardName, cardNumber, name, cardExpireDate} | ||
addCard(newCard) | ||
setCurrentStep("AddCardComplete") | ||
} | ||
|
||
return ( | ||
<div className="button-box"> | ||
<span className="button-text" onClick={handleClick}>다음</span> | ||
</div> | ||
) | ||
} |
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.
c:
- button 태그 대신 div로 구현된 버튼 컴포넌트들이 많이 보이네요. div는 비대화형 요소라 tab으로 선택하려면 tabIndex를 설정해야하고 disabled 속성이 없는 등 버튼 컴포넌트에 적합하지 않아 변경하는게 좋을 것 같습니다.
- 이 컴포넌트도 미션 요구사항에 맞게 bottom up 방식으로 리팩터링 할 수 있을 것 같아요.
const AddCard: React.FC<AddCardProps> = ({cardName, cardNumber, name, expireDate}) => { | |
const {addCard} = useContext(CardContext) | |
const {setCurrentStep} = useStepper() | |
const handleClick = () => { | |
const {month, year} = expireDate | |
const cardExpireDate = `${month} / ${year}` | |
const newCard: CardType = {cardName, cardNumber, name, cardExpireDate} | |
addCard(newCard) | |
setCurrentStep("AddCardComplete") | |
} | |
return ( | |
<div className="button-box"> | |
<span className="button-text" onClick={handleClick}>다음</span> | |
</div> | |
) | |
} | |
const AddCard: React.FC<AddCardProps> = ({cardName, cardNumber, name, expireDate}) => { | |
const {addCard} = useContext(CardContext) | |
const {setCurrentStep} = useStepper() | |
const handleClick = () => { | |
const {month, year} = expireDate | |
const cardExpireDate = `${month} / ${year}` | |
const newCard: CardType = {cardName, cardNumber, name, cardExpireDate} | |
addCard(newCard) | |
setCurrentStep("AddCardComplete") | |
} | |
return ( | |
<Button onClick={handleClick}> | |
<Text>다음</Text> | |
</Button> | |
) | |
} |
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.
Button 추가 하였습니다.
src/components/App.tsx
Outdated
{step === '/' && <AddCardForm/>} | ||
{step === 'AddCardComplete' && <AddCardCompleteForm/>} | ||
{step === 'CardList' && <CardListForm/>} |
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.
r: 카드 입력 폼은 경로처럼 '/'로, 다른 단계는 직접 명시하셨네요. 랜딩 페이지의 느낌으로 '/'를 사용하신 거라 생각되는데, 다른 step과 마찬가지로 적어주는 것이 명시적이고, 또 이후 카드 등록 플로우 요구사항 변경 시 유연하게 대응할 수 있을 것 같습니다.
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.
변경 하였습니다.
src/components/Card.tsx
Outdated
let {first, second, third, fourth} = cardNumber | ||
if (first.length > 0) { | ||
first = first + "-" | ||
} | ||
if (second.length > 0) { | ||
second = second + "-" | ||
} | ||
const thirdSecret = third.length === 4 ? "****-" : "*".repeat(third.length) | ||
const fourthSecret = fourth.length === 4 ? "****" : "*".repeat(fourth.length) | ||
return `${first}${second}${thirdSecret}${fourthSecret}` |
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.
a: 하이픈 추가 시 let, if 대신 함수형으로 로직을 구현하는 것도 고려해볼 수 있습니다.
let {first, second, third, fourth} = cardNumber | |
if (first.length > 0) { | |
first = first + "-" | |
} | |
if (second.length > 0) { | |
second = second + "-" | |
} | |
const thirdSecret = third.length === 4 ? "****-" : "*".repeat(third.length) | |
const fourthSecret = fourth.length === 4 ? "****" : "*".repeat(fourth.length) | |
return `${first}${second}${thirdSecret}${fourthSecret}` | |
const maskString = (input: string) => input.replace(/./g, '*'); | |
const { first, second, third, fourth } = cardNumber | |
return [first, second, maskString(third), maskString(fourth)] | |
.filter((value) => value.length >= 1) | |
.join('-') |
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.
a: 모달 구현 시 다음 사항들도 고려해보세요.
- React Portals API를 사용하면 모달을 DOM 트리에서 분리시켜 깔끔한 구조를 유지할 수 있습니다. (모달이 DOM에서 루트 노드에 렌더링되어 렌더링 문제나 스타일 충돌을 방지)
- 모달의 열린 상태에 따라 페이지의 스크롤을 잠그는 기능등을 훅으로 분리하여 구현하면 추후 재사용에 유리합니다.
src/components/Modal.tsx
Outdated
{chunkedCardNames.map((chunk, index) => ( | ||
<div key={index} className="flex-center"> |
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.
r: 배열을 렌더링할 때 index를 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.
수정하였습니다.
src/components/Modal.tsx
Outdated
{chunk.map((key) => ( | ||
<div className="modal-item-container" | ||
onClick={() => cardSelect(CardType[key as keyof typeof CardType])}> |
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.
r: 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.
수정하였습니다.
src/form/AddCardCompleteForm.tsx
Outdated
function AddCardCompleteForm() { | ||
const {cards} = useContext(CardContext) | ||
const [cardAlias, setCardAlias] = useState('') | ||
const card = cards[cards.length-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.
a: 옵셔널체이닝 혹은 trycatch로도 가능한 점 참고해주세요
const card = cards[cards.length-1] | |
const card = cards?.[cards.length-1] |
src/form/AddCardForm.tsx
Outdated
return ( | ||
<> | ||
<div className="app"> | ||
<HeaderButton className={"page-title"} tag={"h2"} value={"< 카드 추가"} step={"CardList"} event={() => handleClick("CardList")}/> |
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.
a: (참고) 미션 요구사항인 bottom-up 방식으로 구현한다면 레이아웃 화하여 Header, Button, Icon, Title 컴포넌트로 분리해 컴파운드 패턴으로 구현해 재사용성을 높일 수 있습니다.
<HeaderButton className={"page-title"} tag={"h2"} value={"< 카드 추가"} step={"CardList"} event={() => handleClick("CardList")}/> | |
<Button type="button" onClick={() => { | |
setCurrentStep("CardList") | |
}}> | |
<Header tag={"h2"} value={"< 카드 추가"} step={"CardList"} event={() => handleClick("CardList")}> | |
<LeftChevronIcon /> | |
<Title tag="h2">카드 추가</Title> | |
</Header> | |
</Button> |
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.
Bottom up을 장바구니 미션 할 때는 처음부터 한번 해봐야겠네요
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.
안녕하세요 정수님 변경점이 크지 않아 몇가지 사소한 리뷰만 남겼습니다.
스토리북은 공식문서를 따라 구현해보며 학습하는 것을 추천드립니다 😄
{cards | ||
.slice() // 배열을 복사하여 원본 배열에 영향을 주지 않도록 함 | ||
.sort((a, b) => b.id - a.id) // id를 내림차순으로 정렬 |
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.
a: toSorted와 동일합니다.
{cards | |
.slice() // 배열을 복사하여 원본 배열에 영향을 주지 않도록 함 | |
.sort((a, b) => b.id - a.id) // id를 내림차순으로 정렬 | |
{cards | |
.toSorted((a, b) => b.id - a.id) // 배열을 복사 및 id를 내림차순으로 정렬 |
let idCounter = 1 | ||
return Object.values(CardListType).map((cardType) => ({ | ||
id: idCounter++, | ||
cardListType: cardType | ||
})) |
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.
r: map 콜백의 두번째 인자에서 인덱스값을 넘겨줘 let으로 선언하는 대신 사용할 수 있어요.
return Object.values(CardListType).map((cardType, id) => ({
id,
cardListType: cardType
}))
{cardModalTypes.map((card) => ( | ||
<div key={card.id} className="modal-item-container" onClick={() => cardSelect(card.cardListType)}> |
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.
c: cardType 자체가 id로 기능해 따로 id를 만들 필요는 없어 보입니다.
{cardModalTypes.map((card) => ( | |
<div key={card.id} className="modal-item-container" onClick={() => cardSelect(card.cardListType)}> | |
// cardModalTypes는 cardTypes를 배열로 변환한 값으로 수정 | |
{cardModalTypes.map((cardType) => ( | |
<div key={cardType} className="modal-item-container" onClick={() => cardSelect(cardType)}> |
안녕하세요 정민님
페이먼츠 미션 Step1 중간 리뷰 요청합니다.
미션을 완료 한 상황은 아니고, Stepper랑 Storybook 전까지 완료 한 상황입니다. (Card Context에 저장 까지)
Front End 쪽은 실무 경험이 별로 없어서 보시기에 사소한 부분이라도 리뷰해 주시면 감사 할 것 같습니다.
그리고 Stepper 관련 질문이 하나 있습니다.
Stepper 역할을 하는 Component를 직접 구현하는 미션인건지, 아니면 Stepper 검색을 하면 Material-UI Stepper가 많이 나오는 것 같은데
관련 라이브러리를 사용하는 부분인지 궁금합니다.
직접 구현하는 방향이라면 이 부분도 좀 더 공부 해보겠습니다.
Storybook 쪽은 리뷰 해주시는 동안 좀 더 공부해서 리뷰 해주신 부분 + Stepper와 함께 추가 반영 하겠습니다.
혼자 계속 하고 있으면 너무 늦어질 것 같아 먼저 리뷰 요청 드립니다.
페이먼츠 미션 리뷰 잘 부탁드립니다.
감사합니다.