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

[3단계 - 페이먼츠] 호프(김문희) 미션 제출합니다. #152

Merged
merged 18 commits into from
Jun 1, 2022

Conversation

moonheekim0118
Copy link

안녕하세요 YB님! 많이많이 부족하지만..일정상 페이먼츠 3단계 제출합니다 :DD

이전에 정말 좋은 피드백 주셔서 감사드립니다!
다만 ㅠㅠ 일정상 시간이 너무너무 부족해서 피드백을 많이 반영하지 못했고, Context API 사용관련해서도 해답을 아직 찾지 못한 점 알려드립니다. 😭

  • 2단계에 주셨던 카드사명-컬러 상수 관련 피드백 반영하여 적용 하였습니다.

  • 타입스크립트로 몇가지 파일만 마이그레이션 했습니다..!

    • 시간상 모든 파일을 하지는 못했습니다. 😭 😭
    • 아직 타입스크립트 지식이 부족해서, 타입스크립트를 100% 활용하지는 못한 것 같습니다. 😭

먼저, 거의 3~4주 전에 작성했던 리액트 코드를 보니 정말이지 감회가 새웠습니다.. 😮
지금보니 명확하지 않게 작성한 부분이 많았기에 고칠점 투성이라서 사실 처음부터 다시 구현하고싶은 마음이 굴뚝같았어요. 😭
하지만 이러한 코드를 이겨내고 고치는 것 역시 개발자로서 갖춰야하는 중요한 역량이겠죠?!

특히나 제가 컴포넌트를 나누었던 방식이나, Context API 를 활용한 방식을 고치고싶은데,
지금 이 상태에서 고치다보면 구현된 기능을 망가뜨려서 기간을 지키지 못할 것 같아서 멈추고 최소한의 요구사항만 충족하였습니다.

하지만..페이먼츠 코드를 이렇게 방치하고 싶지는 않기에, 다음 미션이 끝나고 개인적으로 현재 페이먼츠 코드에 다음과 같은 것들을 도전해보고자 합니다. (저만의 step4?!)

1. 타입스크립트 마이그레이션 끝내기 & 타입 가드 적용

  • 모든 파일을 타입스크립트로 바꾸고자 합니다.
  • 페이먼츠 어플리케이션 상, 사용자 입력을 받는 기능이 많기에 타입 가드를 적용하여 앱의 안정성을 높이고자 합니다.

2. Context API 활용방법 다시 생각하기

  • 2단계 피드백에서 Context API 활용 방식에 대해서 정확한 답은 없고, 더 나은 방향을 생각해보라고 조언해주셨었는데! 더 나은 방향을 아직 생각하지 못했어요 😭
  • 그리고 당시에 피드백을 받았을 때보다 , 지금 더욱더 Context API 활용 방법이 만족스럽지 않음을 느낍니다 😭
  • 따라서, 이에 대해 더 깊게 고민해보고 현재 Context API 활용 방법에 대해 고민해보고 개선시켜보고자 합니다.

4. 너무 작게 나누어진 컴포넌트 분리에 대해서 다시 생각하고 리팩토링하기

  • 현재 재사용되지도 않으면서, 작게 나누어지기만 한 컴포넌트들이 몇가지 보입니다. 이것들을 없애고 조금 더 큰 단위의 컴포넌트들로 리팩토링해보고자 합니다.

2단계로부터 개선되지 못한 코드에 대해 리뷰요청을 보내게 되어 많이 죄송스럽습니다 😭
꼭! 다음 미션이 끝나면 혼자서 묵묵히 페이먼츠로 돌아와서 위의 고민거리를 하나씩 쳐내 보려구합니다.

레벨1 미션에서도 그렇고, 페이먼츠 step1, step2에서 정말 좋은 리뷰 감사드렸습니다 🙇 🙇
YB님의 리뷰 덕분에 정말 많이 고민하고 성장 할 수 있었습니다 : ) 👍 YB님도 레벨2 남은 리뷰 화이팅하시길 바랍니다 👍 👍

@pocojang pocojang self-requested a review May 28, 2022 23:17
@pocojang pocojang self-assigned this May 28, 2022
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.

안녕하세요
홒 대신 왔습니다

TS 피드백만 몇가지 남겨볼게요

config

몇가지 설정이 제대로 검증되어 있지 않거나 확인되지 않은 것으로 보입니다!

일관성

타입에 일관성이 부족합니다
예를 들어 T- prefix 와 같은 아이디어처럼 일관성을 지키면서 타이핑해보세요 :)

그나저나 Prop Types & 스토리북은 이제 제거하시는건가요?!

페이먼츠 미션 고생 많으셨어요
이제 정말 놓아줍시다
스텝4...! 기대해도 되나요?

Comment on lines +70 to 75
"@typescript-eslint/eslint-plugin": "^5.26.0",
"@typescript-eslint/parser": "^5.26.0",
"chromatic": "^6.5.4",
"eslint": "^8.14.0",
"eslint-plugin-react": "^7.29.4",
"eslint-plugin-react-hooks": "^4.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,3 @@
{
"typescript.tsdk": "node_modules/typescript/lib"
Copy link
Contributor

Choose a reason for hiding this comment

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

꼭 필요한 설정이 맞을까요?

return (
<Styled.Container onClick={onClick}>
<Styled.EmptyCard color={CARD_COLOR_BY_NAME[cardType]}>
<Styled.EmptyCard color={CARD_TYPE[cardType]?.color ?? 'white'}>
Copy link
Contributor

Choose a reason for hiding this comment

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

'white' 또한 관리하는게 어떨까요?

const FieldSet = ({ id, description, children, errorMessage, isError }) => {
interface Props {
id: string;
children: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

이건 PropsWithChildren사용하지 않으시나요?!

children: React.ReactNode;
}

const Header = ({ children }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 타입으로 대체되지 않을까요?

Comment on lines +7 to +8
export const ENDPOINT =
'https://moonheekim-payments-server.herokuapp.com/cards';
Copy link
Contributor

Choose a reason for hiding this comment

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

env로 제거해도 좋겠어요~

<Button type="link" to="/">
🏠 홈으로 가기
</Button>
<Link to="/">🏠 홈으로 가기</Link>
Copy link
Contributor

Choose a reason for hiding this comment

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

PATH 재활용이 되지 않네요~

firstPassword,
secondPassword,
cardType,
}: Args): ReturnType => {
Copy link
Contributor

Choose a reason for hiding this comment

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

마치 모든 유틸을 다 보관할 것 같은 파일인데 유틸리티 같은 타이핑을 제공하시네요!

@@ -1,41 +1,40 @@
import { REG_EXP } from '../constant';

Copy link
Contributor

Choose a reason for hiding this comment

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

대부분의 코드들은 추론하는데 문제 없어보입니다 :)

Comment on lines +6 to +7
to: string;
state: unknown;
Copy link
Contributor

Choose a reason for hiding this comment

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

React Router 타이핑도 찾아보세요 :)

@pocojang pocojang merged commit 0428ecb into woowacourse:moonheekim0118 Jun 1, 2022
@youngbeomrhee
Copy link

youngbeomrhee commented Jun 2, 2022

호프님 안녕하세요 미션 수행하느라 정말 고생 많으셨어요~
코드레벨에 대해서는 포코님이 코멘트 주셔서 저는 기존에 이야기 나눴던 부분에 대해서만 추가 피드백 드릴께요
코드를 다시 봐도 여전히 Context API에서 일시적인 상태값을 보관해야 되는가?가 의문이네요
일단 첫 번째 개선이 필요한 부분은 isInitial-으로 정의한 상태값들은 AddCardForm 컴포넌트에서만 사용되는 것으로 보여요
그런데 불필요하게 전역에서 관리되고 있어서 AddCardForm 내부의 state로 옮기고 전역에서는 제거되면 좋을 것 같아요
이렇게만 해도 불필요한 is-의 절반이 삭제되네요

그 외의 is- 로 시작하는 일시적인 상태값도 꼭 전역에서 관리될 필요가 있는 상태인자 다시 고민해 보시고 꼭 필요한 구조가 되었다면 적절한 위임이 이루어지지 않아서 불필요한 의존성이 생긴것은 아닌지 점검해 보시면 좋을 것 같아요
실시간 검증을 하고 싶다면 입력값이 바뀔때마다 검증하고 값에 문제가 있다면 전달 받은 error handler를 호출만 하는 구조면 충분하지 않나 싶어요

말씀드린 내용 다시 한 번 고민해 보시면 좋을 것 같고요
긴 시간 동안 미션 수행과 더 나은 코드를 위해 고민하고 개선하느라 고생 많으셨어요
남은 미션 잘 마무리하시고 좋은 결과 있으시길 응원할께요 :)

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.

3 participants