Skip to content
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

[2단계 - 페이먼츠] 밧드(감우영) 미션 제출합니다. #120

Merged
merged 46 commits into from
May 11, 2022

Conversation

kamwoo
Copy link

@kamwoo kamwoo commented May 7, 2022

안녕하세요! 아래 데모페이지와 스토리북 링크입니다.
데모페이지
스토리북


구조

image

  • Context와 Reducer를 같이 사용했습니다. InputtedInfoReducer는 폼에서 입력값들이 CardData로 보내기 전까지 저장하기 위해 사용됩니다. CardReducer는 카드들 정보 리스트를 조작하는데 사용됩니다.

  • 카드 추가 input에서 입력을 하면 inputtedData에 저장됩니다.
    카드 추가 폼에서 submit을 하면, 카드 이름 미입력 상태로 cardData에 저장됩니다.
    카드 이름 폼에서 submit을 하면, inputtedData로 cardData에서 해당하는 카드에 이름을 추가합니다.
    inputtedData를 초기화합니다.

  • url로 Complete페이지나 Add페이지에 접근할 때 오류를 방지하기 위해 add페이지는 inputtedData를 마운트될 때 초기화되고, Compelete페이지는 alert와 List페이지로 바로 이동하게 만들었습니다. 배포를 하면서 지금은 url로 접근이 안되기 때문에 굳이 필요는 없을 것 같습니다.


질문

  • 1단계에서 말해주신 수정에 용이한 컴포넌트를 만들어 볼려고 했습니다. 리액트에서도 ui로직과 도메인 로직을 분리하면 관심사를 분리해서 관리하기 수월해진다는 글을 보고 적용해봤습니다. 분리를 하다보니 컴포넌트 하나당 커스텀 훅이 생기게 되었는데, 만약에 컴포넌트가 엄청 많아진다면 커스텀 훅도 같이 많아져야되고, 1:1 이면 분리를 하는 이유가 없을 것 같아서 고민이 생겼습니다. 리트님은 분리하는 것에 어떻게 생각하시는 지와 지금 구조에서 개선되었으면 하는 것이 있는지 궁금합니다!

kamwoo added 30 commits May 4, 2022 13:05
- useContext를 사용하려는 목적으로 카드 컴포넌트를 포함한다.
- 카드 정보를 불러와 카드에 props로 전달
- browerRouter를 감싸는 최상단으로 위치 이동
Copy link

@lsw1164 lsw1164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1단계에서 말해주신 수정에 용이한 컴포넌트를 만들어 볼려고 했습니다.
리액트에서도 ui로직과 도메인 로직을 분리하면 관심사를 분리해서 관리하기 수월해진다는 글을 보고 적용해봤습니다.
분리를 하다보니 컴포넌트 하나당 커스텀 훅이 생기게 되었는데,
만약에 컴포넌트가 엄청 많아진다면 커스텀 훅도 같이 많아져야되고,
1:1 이면 분리를 하는 이유가 없을 것 같아서 고민이 생겼습니다.
리트님은 분리하는 것에 어떻게 생각하시는 지와 지금 구조에서 개선되었으면 하는 것이 있는지 궁금합니다!

안녕하세요 밧드님, 코드 잘봤습니다! 분리 시도하신 건 좋았습니다.
하지만 말씀하신대로 커스텀 훅이 컴포넌트 마다 생기는 건 과하다는 생각이 드실겁니다.
'커스텀 훅을 잘 쓰는 건 로직을 적절하게 추상화해서 재사용 가능하도록 추출' 이라고 들어보셨나요?
재사용 가능하도록 추상화 할 곳은 없을지 고민해보시면 좋을 거 같습니다.
지금 작성하신 컴포넌트 중에서 훅을 쓰지 않는 게 오히려 구조적으로나 가독성 측면에서나 더 좋다고 생각하는 컴포넌트가 있을까요?

개선할 점

  • 카드 유효 기간 입력이 이상합니다.
    (1~12월만 받을 수 있게 수정 필요)

  • 카드 소유자 이름이 선택인데 반드시 입력해야 카드가 만들어 집니다.

분리에 대한 생각

컴포넌트를 분리해서 중간에 다른 컴포넌트를 두거나 hook을 만들거나 이런 과정들이 결국 Depth를 늘리는거죠?
설계에서 Depth를 늘리는 건 유연함을 얻는 대신 구조의 복잡도가 올라간다고 생각합니다.
(만능 설계란 없고 트레이드 오프라는 것을 염두하셔야 됩니다)

응집도 높은 로직들을 한 곳에 짰는데 변경 사항도 없고 읽기도 좋다? 그럼 베스트입니다.

그런데 특정 요구 사항이 변경이 잦고 앞으로도 변경이 예상된다?
-> 분리를 고려합니다. (잘 분리할 수 있도록 다양한 패턴이나 구조를 학습하는 겁니다.)
공통적인 부분을 추상화해서 추출해야 유연한 설계가 됩니다.
(개발하는 서비스 & 도메인에 대한 이해가 필요)

유연하게 설계하려면 복잡도가 올라가는 만큼 파악하기 쉽지 않습니다.
따라서 의도 전달을 최대한 돕기 위해 네이밍이 더 중요해집니다.

Comment on lines 32 to 37
<Fragment
key={cardInfo.cardNumber.value.fist + cardInfo.cardName.value}
>
<Card {...cardInfo} />
<span>{cardInfo.cardName.value}</span>
</Fragment>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리려는 UI 단위를 확실히 구분하려면 Fragment 보다 Wrapper 컴포넌트가 있으면 좋을 거 같습니다.
추후 CSS 조절이 필요할 때 Fragment로 하기 힘들 거 같습니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 카드 리스트 페이지에서만 사용하고 css조절을 위해서 만드는 거라고 생각해서 styled component로 만들어서 추가했습니다.

const { inputtedInfo, inputtedInfoDispatch } = useContext(CardContext);

useEffect(() => {
if (Object.keys(inputtedInfo).length !== 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0이 아닐때만 클리어 하는 이유가 있을까요?

Copy link
Author

@kamwoo kamwoo May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 페이지로 이동해도 inputtedInfoData가 그대로 유지된다고 생각해서 useEffect을 만들었는데 확인을 해보니 없어도 useSomeInput이라는 커스텀 훅이 재실행되면서 초기화를 시켜줘서 해당 useEffect는 삭제했습니다

import { useNavigate } from 'react-router-dom';
import { LINK } from '../../constant/Link';

function useCard({ cardNumber, expiredDate, ownerName }) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useCard는 어떤 역할이라고 보면될까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

객체인 데이터를 문자열 형태로 만들어서 리턴해주고,
카드를 클릭했을 때 카드 추가 페이지에서 입력을 다하고 submit을 눌렸을 상황과 동일하게 만든 다음, 카드 추가 페이지로 이동하도록 만드는 역할을 합니다.

isValidForm ? alert(MESSAGE.SAVE_INFO) : alert(MESSAGE.DENY_SAVE);
};
function CardAddForm({ link }) {
const { isValidForm, onAddFormSubmit } = useCardAddForm(link);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onAddFormSubmit - Add, Submit 이라는 동사가 둘 다 들어가있어서 헷갈립니다. 개선할 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onAddFormSubmit에서 onCardInfoSubmit으로 변경해봤습니다.

const { inputtedInfo } = useContext(CardContext);

useEffect(() => {
if (Object.keys(inputtedInfo).length === 0) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

카드 만들기 완료 후에 뒤로 한 번 누르니까 alert이 뜨며서 List 페이지로 가지는 거 확인했습니다.
그런데 거기서 한 번 더 뒤로가기 누르니까 Complete 페이지로 접근됩니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

카드 완료 페이지가 마운트 되고 inputtedData에 carName으로 빈값으로 넣는 로직 때문인 것으로 확인했습니다. 계획상으로 이름을 입력하지 않았을 때 "이름 없음"을 보여줄려고 만들었던 거라 이 부분을 수정하는 대신에 카드 리스트 페이지가 마운트 될 때 inputtedData를 비워주는 방법으로 해결했습니다.

Comment on lines 13 to 14
.map(key => inputtedInfo[key].isValid)
.every(isValid => isValid);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map 없이 every만 써도 되지 않을까요?

Object.values(cardInfo.cardNumber.value).join('')
);

return cardNumberList.find(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find가 어떤걸 반환하고 있죠? checkExistCard는 bool값 반환을 의도한 게 아닐까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 적절한 메소드로 수정했습니다.!

const inputtedCardNumber = Object.values(inputtedInfo.cardNumber.value).join('');

return cardNumberList.includes(inputtedCardNumber);

@kamwoo
Copy link
Author

kamwoo commented May 10, 2022

안녕하세요 리트님! 기능 안되는 거 피드백 적용했습니다.
추가로 이번에 container를 적용해봤고 container 로직에서 세부적인 구현들은 커스텀 훅으로 빼는 방식을 적용해봤습니다.
기존에 컴포넌트에서 내부 로직을 전부 커스텀 훅으로 빼면서 고민했던, 필요성이나 재사용성을 어느정도 해결한 것 같다고 느꼈습니다.
부족한 점이나 시도해봤으면 하는 것이 있으면 피드백 부탁드립니다!

Copy link
Contributor

@pocojang pocojang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰 요청 마감일이 지났기때문에 PR 머지하겠습니다
리뷰어와의 코멘트는 소통은 가능합니다.
이후 미션에 대한 완주 의지가 있다면 일단 장바구니 미션에 집중하시고 다음 스텝3에서 수행해주세요 :)

@pocojang pocojang merged commit b9250d0 into woowacourse:kamwoo May 11, 2022
@kamwoo kamwoo deleted the step2 branch May 24, 2022 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants