-
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기 한재성] 페이먼츠 미션 Step2 #140
base: han-d-peter
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,2 번 정도로만 심플하게 나눠보면 어떨까 싶어요. 지금의 폴더구조를 봤을 때 하위 폴더들이나 컴포넌트들의 기준도 조금 애매해보이네요(A page하위의 컴포넌트가 B page 에서 사용된다던지..)
입력을 중간에 중단한 상태에서 새로 시작하기 위한 진입이 어려웠습니다..
이 케이스에 대해 조금만 구체적으로 설명주시면 도움드려볼 수 있을 것 같아요!
컴포넌트 내부 상태를 참고 하고 있는 함수를 여러개로 쪼개 들고 있게 하는것이 옳은지 고민입니다. 상태를 참조하고 있기 때문에 분리된 함수가 해당 컴포넌트를 벗어날 수 없기 때문입니다
상태를 업데이트 하거나 상태를 강하게 의존하는 아이들을 결국 그 컴포넌트 안에서 사용될 목적(주로 이벤트 핸들러들이 그렇겠죠?)이 크게 때문에 그리 부자연스럽지는 않은 것 같습니다. 다만 꼭 특정 컴포넌트에서 선언되고 사용될 필요없는 재사용성이 큰 함수들은 사이드 이펙트 없이 인자를 받아 할 일을 수행하는 순수함수로 작성해보시면 좋을 것 같아요.
const copied = [...cards]; | ||
const targetCard = copied.find(isThis); | ||
if (targetCard) targetCard["holderName"] = name.cardName; | ||
setCards(copied); | ||
navigate("/mycards"); |
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.
지금의 방식도 좋지만 Array.prototype.map같은 내장 메서드를 활용하면 더 선언적으로 작성할 수 있을 것 같아요!
const newCards = cards.map(card =>
isThis(card)
? { ...card, holderName: name.cardName }
: card
)
setCards(newCards)
const { cards, setCards } = useCards({ | ||
sortByKey: "createdAt", | ||
sortMethod: "asc", | ||
}); | ||
|
||
const latest = cards.sort((a, b) => { | ||
return new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime(); | ||
}); |
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.
오 useCards에 sort option을 주고있어서 알아서 정렬을 할줄 알았는데 한 번 더 해야하나요?
sortMethod: "asc", | ||
}); | ||
|
||
const latest = cards.sort((a, b) => { |
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.
sort 메서드는 원본도 바꿉니다 사용에 유의해주세요
1. 컴포넌트 분류
컴포넌트를 세가지 관점에서 분류해봤습니다
이 프로젝트가 아니더라도 사용할 수 잇는 컴포넌트
이 프로젝트 안에서 공유될 수 있는 컴포넌트
특정페이지에서만 사용할 수 있는 컴포넌트
위 세가지로 분류했을때 처음엔 3번이었다가 2번으로 이동하는 경우가 많았습니다.
그렇다고 모든 컴포넌트를 모아두기엔 나중을 생각한다면 너무 어지러워져서 고민입니다.
2. step에 따른 정보 누적
step이 진행됨에 따라 부모의 상태가 정보를 누적하도록 했습니다.
다만 이렇게 되면 새로고침 상황에서 상태가 휘발되어 버리는 문제가 있었습니다.
로컬스토리지를 통해 상태를 저장하면 새로고침을 해도 진행할 페이지가 유지되지만
입력을 중간에 중단한 상태에서 새로 시작하기 위한 진입이 어려웠습니다..
3. 선언적 프로그래밍
코드 정리가 어려웠는데
시간이 부족했고
컴포넌트 내부 상태를 참고 하고 있는 함수를 여러개로 쪼개 들고 있게 하는것이 옳은지 고민입니다. 상태를 참조하고 있기 때문에 분리된 함수가 해당 컴포넌트를 벗어날 수 없기 때문입니다
4. 테스트
이번 테스트는 storybook 에서 제공하는 Play 를 사용했습니다.
기본단위 컴포넌트
도메인에 종속된 작은 단위 컴포넌트
조립된 컴포넌트(는 시간이 없어서 ㅠㅠ PR 올리고 작업중입니다)
필수 요구사항
카드 추가 확인
카드 목록