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

[페이먼츠 미션 Step 2] 윤생(이윤성) 미션 제출합니다. #258

Merged
merged 102 commits into from
May 5, 2023

Conversation

2yunseong
Copy link

@2yunseong 2yunseong commented Apr 29, 2023

안녕하세요 파랑!

이번 미션은 Custom Hook과 재사용 가능한 컴포넌트를 고민하며 구현했습니다. 거기에 이제 Context API 한 숟갈을 얹어서.. 공부해보았어요!

Custom Hook & 재사용가능한 컴포넌트

그러면서 자연스럽게 기존의 구조를 좀 많이 바꾸었는데요.. 특히 Hook 부분과 Input창 들을 많이 고민해보았습니다. 기존 설계로는 사용자가 유효한 값을 입력하지 않았을 때, 에러 메세지를 띄우기가 어렵더라구요.
아래는 고민을 하며 정리한 글이에요. 링크

ContextAPI

거기에 Context API 를 사용하였습니다. 카드 등록을 다 마친 후에 별칭을 따로 등록하는데(이하 별칭 등록), 이 때 이전에 등록된 카드 데이터를 보여주어야 되어서 별칭등록 페이지에서도 카드 정보가 필요했어요. 그래서 Context API를 사용하였습니다.

질문이자 사과의 말씀 올립니다

비즈니스 로직은 디렉토리 상 어디에 위치해 놓을 지 고민입니다.. 그래서 기능구현에 마구잡이로 생성해서 쓰다보니.. 좀 이것저것 난잡한 경향이 있는데 어떻게 관리하면 좋을까요? (고민인 부분에 커멘트 달아놓겠습니다)

배포 사이트

페이먼츠

- try 문 안에서 처리되어야 하는 로직 합쳐줌
@2yunseong
Copy link
Author

2yunseong commented May 3, 2023

안녕하세요 파랑~! 오늘 그래도 어느정도 한 것 같아서 요청 보내려고 했는데, 먼저 댓글 남겨주셔서 감사합니다!! :) 이미 댓글로 피드백은 받았지만, 요청은 늦게나마.. 눌러봅니다.

리뷰요청 보내는 김에 파랑이 제시해주신 문제점에 남긴 질문이 있는데 한번 검토 부탁드립니다!
즐거운 저녁 보내세요~!!😄

@2yunseong
Copy link
Author

파랑 안녕하세요! 다시 리뷰요청 드립니다!
좀 고민을 하다.. 컨텍스트 API를 삭제하는 방향으로 갔습니다.

즐거운 연휴 보내세요!!!! 🥹

Copy link

@InSeong-So InSeong-So left a comment

Choose a reason for hiding this comment

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

리팩토링된 코드가 엄청 많네요...👍
구조적인 문제를 많이 고민하는 것 같아요.
좋은 태도인데, 스스로 주화입마에 빠지지 않도록 기준을 잘 잡으면 좋겠습니다. 과하진 않은지, 결국 무엇을 해야하는지, 미션의 의도가 무엇인지... 꾸준히 생각하고 이를 리스트업한 뒤에 개발하는 것을 추천드려요.

useInput에 대한 테스트 코드가 너무 인상 깊네요. 이런 부분은 윤생의 큰 장점인 것 같습니다. 유저 액션에 대한 대응을 철저하게 하려는 자세 말이죠.

라우팅 코드 피드백이 적용된 것을 봤는데, 확실히 이해하기 좋았어요. 다만 아쉬운 것은, 공유해준 피그마에서 3번째 after는 미션 본질을 벗어나진 않았을까 싶습니다. 딱 2번째 리팩토링된 내용이 Context API를 활용하고 이에 어떻게 초깃값을 세팅할지 고민한 흔적이 있어 좋았거든요.

코드량, 파일 갯수가 많은 이유는 그만큼 다양한 걸 시도하고 사고했다는 이야기라 칭찬을 많이 하고 싶습니다. 피드백 적용하느라 고생 많았어요.

나머지는 코멘트에서 그 결만 확인하고, 스탭 3에서 봅시다!
머지하도록 할게요. 그리고 윤생은 한 번 페어 프로그래밍을 같이 진행해보면 좋겠네요☺️

src/utils/card.ts Show resolved Hide resolved
src/utils/card.ts Show resolved Hide resolved
src/utils/card.ts Show resolved Hide resolved
src/utils/card.ts Show resolved Hide resolved
src/test/hooks/useInput.test.ts Show resolved Hide resolved
src/router/AppRouter.tsx Show resolved Hide resolved
src/pages/CardList/index.tsx Show resolved Hide resolved
src/pages/CardAlias/index.tsx Show resolved Hide resolved
src/pages/AddCard/domain/validation.ts Show resolved Hide resolved
@InSeong-So InSeong-So merged commit 4f380ef into woowacourse:2yunseong May 5, 2023
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.

2 participants