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 1] 윤생(이윤성) 미션 제출합니다. #205

Merged
merged 61 commits into from
Apr 25, 2023

Conversation

2yunseong
Copy link

안녕하세요 파랑! 윤생입니다~!

이번 미션은 3주네요~! 3주간 잘 부탁드립니닷!!

이번 미션은 스토리북을 통한 컴포넌트 주도 개발을 중점적으로 잡아보려 했는데, 제한 시간 내에 문제에 대한 기능 요구사항도 전부 구현하지 못할 것 같아서 일단 스토리북 사용 자체도 뒤로 미루었습니다 ㅠ (페어와 저와 모두 처음이였어서요) 남은 기간 동안 천천히 적용해보겠습니다.

질문

Q. 수업에서 배운 내용으로는 컴포넌트 주도 개발은 컴포넌트 단위로 빠른 피드백을 받는 것이 목적이라고 배웠습니다. 그래서 저는 스토리북을 통해 자신이 개발한 컴포넌트를 빠르게 시각적으로 보고 테스트할 수 있다는 점이 그렇다고 생각하는데, 제가 생각하는 방향이 맞을까요?

Q. 처음에 input 태그를 만들 때 value나 onChange등 몇개의 prop만 필요할 줄 알았는데, 앱이 점점 커지면서 필요한 props도 많아지더라구요. 이건 어쩔 수 없는 부분인 걸까요? 왠지 props 가 기존에 설계했던 것보다 늘어날수록 잘못 설계한게 아닌지 하는 생각이 들어서요.

Q. 컴포넌트 네이밍이 어렵습니다.ㅠ 파랑만의 컴포넌트 네이밍 방식이 있으신가요? 혹시 보시다가도 별로인 네이밍이 있다면 피드백 부탁드리겠습니다.

미 충족 사항

  • 만료일 입력이 에러가 납니다. 1이나 0을 입력하고 다시 지우면 안지워지는 문제가 있어요.
  • 카드 소유자 이름에서 공백을 발생시키면 카드가 세로로 늘어나는 문제가 있습니다.
  • 카드 소유자 이름은 선택이지만 필수로 동작합니다.

배포 링크

이번 미션은 모바일 웹/앱입니다! 앱 으로써의 접근성이 떨어지는 부분도 피드백 해주시면 감사드리겠습니다!!

페이먼츠 바로가기

Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

몇 가지 질문 코멘트로 남기었습니다!

src/components/FormCardAdd.tsx Outdated Show resolved Hide resolved
name: string;
};

const InputCardData = ({ value, onChange, name, className }: InputProps) => {
Copy link
Author

Choose a reason for hiding this comment

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

아래 InputCardPassword와 사실상 같습니다. 원래는 다른 방향으로 구현될 것 같아 다른 컴포넌트로 생각했습니다. 그래서 합치려고 하다 마감시간이 다가왔네요..! ㅠㅠ
그런데 의문이 드는게 있습니다! 이렇게 단일 input을 컴포넌트화 하는게 맞을까요? 아니면 제 접근방식이 잘못된건가요?

Choose a reason for hiding this comment

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

단일 input도 공통적인 스타일(디자인 시스템 input 컴포넌트 같은)이 존재한다면 분리하는 게 맞습니다. 단, 이 경우 input 태그의 자유도를 높이기 위해서는 기본 attributes를 주입받을 수 있게 해줘야 하므로 React에서 제공하는 타입을 활용해보길 추천드려요.

Copy link
Author

@2yunseong 2yunseong Apr 24, 2023

Choose a reason for hiding this comment

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

단, 이 경우 input 태그의 자유도를 높이기 위해서는 기본 attributes를 주입받을 수 있게 해줘야 하므로 React에서 제공하는 타입을 활용해보길 추천드려요.

혹시 추천해주실만한 검색 키워드가 있나요?! 스스로 검색을 해보는데 해당 내용에 대해 잘 나오지 않네요 ㅠㅠ
이어서 기존에 input만 감싸던 컴포넌트를 카드번호/비밀번호/cvc/유효일 등으로 변경해서 아쉽게 코드에 적용은 못하지만, 한번 공부해보겠습니다!

Choose a reason for hiding this comment

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

HTMLInputElement 타입과, InputHTMLAttributes 타입을 적용해보길 추천드려요!

@InSeong-So InSeong-So self-requested a review April 21, 2023 06:54
@InSeong-So InSeong-So self-assigned this Apr 21, 2023
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.

안녕하세요, 윤생! 리뷰어 파랑입니다🤗
긴 미션 동안 잘 부탁드려요! 많은 도움이 되면 좋겠네요💪

Q. 수업에서 배운 내용으로는 컴포넌트 주도 개발은 컴포넌트 단위로 빠른 피드백을 받는 것이 목적이라고 배웠습니다. 그래서 저는 스토리북을 통해 자신이 개발한 컴포넌트를 빠르게 시각적으로 보고 테스트할 수 있다는 점이 그렇다고 생각하는데, 제가 생각하는 방향이 맞을까요?

  • 개발을 하다보면 꼭 특정 상황에 의존하는 UI가 등장합니다. 로그인을 했을 때 보이는 컴포넌트라던가... 말이죠! 스토리북은 이런 상황에서 props를 유저가 직접 조작하면서 컴포넌트를 확인할 수 있는 장점이 있습니다. 개발자 뿐만 아니라 PM, PD, QA가 전부 가능하죠. 따라서 윤생이 생각한 것도 좋은 방향이랍니다☺

Q. 처음에 input 태그를 만들 때 value나 onChange등 몇개의 prop만 필요할 줄 알았는데, 앱이 점점 커지면서 필요한 props도 많아지더라구요. 이건 어쩔 수 없는 부분인 걸까요? 왠지 props 가 기존에 설계했던 것보다 늘어날수록 잘못 설계한게 아닌지 하는 생각이 들어서요.

  • 잘못 설계한 것이 아니라, 그 상황에서는 그게 옳았을 뿐입니다. props가 많아져 확장을 해야하는 상황이 온다면 그 시점에 리팩토링을 하거나, 기술부채를 남기고 기능 구현을 우선하는 방법 중에 선택하는 것이죠. 일반적으로 네이티브 태그가 가진 속성에 대한 props 설계는 크게 변하는 일이 없지만, 요구사항에 따른 도메인 종속적인 컴포넌트는 천차만별 바뀌기 때문에 많은 컴포넌트를 개발하고 확장의 여지를 남겨두는 경험을 쌓는 것이 좋습니다.

Q. 컴포넌트 네이밍이 어렵습니다.ㅠ 파랑만의 컴포넌트 네이밍 방식이 있으신가요? 혹시 보시다가도 별로인 네이밍이 있다면 피드백 부탁드리겠습니다.

  • 저는 공통 컴포넌트라면... Todo App이나 날씨 앱에 두었을 때 이상하지 않는가? 를 기준으로 작성합니다. 우선 Header, Footer, Sidebar, Modal 등이 있겠네요. 도메인이라면 templates 라는 폴더 하위에 페이지(라우팅) 이름으로 하나하나 스탭을 맞춰가면서 작성합니다. /add-card 가 라우팅이라면 AddCardTemplate > AddCardWrapper > AddCardForm > AddCardButton 처럼 되겠네요!

스토리북은 처음 접할 때 어려움이 많아 포기하는 분들이 많습니다. 이걸 이겨내면 여러 컴포넌트에 대한 시각적 접근, 설계에 대한 고민, 빠른 수정과 피드백을 받을 수 있는 창구를 만들어 낼 수 있으므로 꼭 이겨내길 바래요.

코멘트 확인해보시고, 재요청주세요!

src/components/Card.tsx Outdated Show resolved Hide resolved
src/components/FormCardAdd.tsx Outdated Show resolved Hide resolved
src/components/Header.tsx Show resolved Hide resolved
name: string;
};

const InputCardData = ({ value, onChange, name, className }: InputProps) => {

Choose a reason for hiding this comment

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

단일 input도 공통적인 스타일(디자인 시스템 input 컴포넌트 같은)이 존재한다면 분리하는 게 맞습니다. 단, 이 경우 input 태그의 자유도를 높이기 위해서는 기본 attributes를 주입받을 수 있게 해줘야 하므로 React에서 제공하는 타입을 활용해보길 추천드려요.

src/components/InputCardPassword.tsx Outdated Show resolved Hide resolved
src/index.tsx Outdated
]);

const root = ReactDOM.createRoot(document.getElementById('root') as HTMLElement);
root.render(<RouterProvider router={router} />);

Choose a reason for hiding this comment

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

StrictMode가 없네요😳

router 코드는 가급적 다른 파일에 분리해서 적용해주는 게 좋습니다. entry 파일은 최대한 가볍게 유지해주는 것을 추천드려요.

Copy link
Author

Choose a reason for hiding this comment

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

### Strict 모드
- 자손들에 대한 부가적인 검사와 경고를 활성화 한다.
- Strict 모드는 개발모드에서만 활성화

확실히 적용하니 key 누락 등의 오류가 보이네요!! 감사합니다!

entry 파일이 가볍게 유지되야 하는건 어떤 이유에서인가요?

Choose a reason for hiding this comment

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

가볍다라는 것은 용량적인 크기가 아니라, 관심사의 크기입니다. 현재는 CRA로 만들어진 앱이라 index.tsx, App.tsx 가 존재하게 되는데요, 우리는 App.tsx에서 전역적인 컴포넌트나 라우팅, 스토어 등을 설정합니다. 왜냐하면 JSX 구문이 깊어지고 실제 프로젝트가 구동될 때 설정을 총동원하는 파일이라서 그럽니다.

반면 index.tsx는 리액트가 "구동되기 위한" 단계로써 해당 프로젝트에 필요한 정적 리소스 등을 셋업하는 과정이기 때문에, 명확한 관심사의 분리가 필요하다는 관점에서 설명 드린겁니다~!

Copy link
Author

Choose a reason for hiding this comment

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

관심사의 크기라는 말이 마음을 울리네요..!! 다음부터는 그런 관점으로 접근해보겠습니다!!

src/pages/AddCardPage.tsx Show resolved Hide resolved
src/pages/AddCardPage.tsx Outdated Show resolved Hide resolved
src/pages/CardListPage.tsx Show resolved Hide resolved
src/utils/fetchData.ts Outdated Show resolved Hide resolved
@2yunseong
Copy link
Author

안녕하세요 파랑~ 일단 코멘트 주신 내용 궁금한 점이나 논의하고 싶은 점 바탕으로 어느정도 한 것 같아 한번 더 제출합니다! 물론 아직 미흡한 점이 많지만, 지금까지 공부한 것이랑 생각한 것에 대해 피드백 받고 싶어 리뷰 요청 보내드립니다! 감사합니다 :)

Comment on lines +25 to +34

const AddInputHooks = () => {
const cardExpireDate = useInput('', cardExpireCondition, formatExpireDate);

return <AddCardExpireDateInput cardExpire={cardExpireDate} />;
};

export const Primary = {
render: () => <AddInputHooks />,
};
Copy link
Author

Choose a reason for hiding this comment

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

스토리북에서 onChange등의 동작을 확인하기 위해 이런식으로 사용하는게 맞을까요?
참고한 링크입니다.

Choose a reason for hiding this comment

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

넵넵 잘 접근하셨어요!

Choose a reason for hiding this comment

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

이렇게 진행할 때, AddInputHooks에 props를 동적으로 할당해야 한다면 어떻게 할 지 고민해보면 좋겠네요!

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.

아이고, 리뷰가 늦었네요!
궁금증에 대한 답변도 달았고, 리뷰도 남겨두었으니 확인바랍니다!

좀 더 핑퐁을 하는 것보다, 크게 재요청 하지 않고 스탭2를 진행하면서 점진적으로 고치는 것이 더 좋을 것 같아요.
따라서 이만 머지하도록 할게요!

궁금한 내용, 이해가 되지 않는 부분은 스탭2에 이어서 진행하면 좋겠습니다!

Comment on lines +25 to +34

const AddInputHooks = () => {
const cardExpireDate = useInput('', cardExpireCondition, formatExpireDate);

return <AddCardExpireDateInput cardExpire={cardExpireDate} />;
};

export const Primary = {
render: () => <AddInputHooks />,
};

Choose a reason for hiding this comment

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

넵넵 잘 접근하셨어요!

.storybook/main.ts Show resolved Hide resolved
Comment on lines +6 to +18
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/react": "^13.4.0",
"@testing-library/user-event": "^13.5.0",
"@types/jest": "^27.5.2",
"@types/node": "^16.18.23",
"@types/react": "^18.0.37",
"@types/react-dom": "^18.0.11",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-router-dom": "^6.10.0",
"react-scripts": "5.0.1",
"typescript": "^4.9.5",
"web-vitals": "^2.1.4"

Choose a reason for hiding this comment

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

CRA든, CVA든, CNA든 항상 의존성 파일을 유심히 보도록 해요!

Comment on lines +9 to +10
value={cardExpire.value}
onChange={cardExpire.onChange}

Choose a reason for hiding this comment

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

value, onChange는 AddCardExpireDateInput 내부에 두는 것도 좋을 것 같네요!
꼭 props를 할당할 필요는 없어보입니다.

src/components/AddCardForm.tsx Show resolved Hide resolved
src/pages/AddCardPage.tsx Show resolved Hide resolved
src/pages/cardInputCondition.ts Show resolved Hide resolved
src/router/AppRouter.tsx Show resolved Hide resolved
Comment on lines +25 to +34

const AddInputHooks = () => {
const cardExpireDate = useInput('', cardExpireCondition, formatExpireDate);

return <AddCardExpireDateInput cardExpire={cardExpireDate} />;
};

export const Primary = {
render: () => <AddInputHooks />,
};

Choose a reason for hiding this comment

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

이렇게 진행할 때, AddInputHooks에 props를 동적으로 할당해야 한다면 어떻게 할 지 고민해보면 좋겠네요!

@InSeong-So InSeong-So merged commit 62cbb78 into woowacourse:2yunseong Apr 25, 2023
Copy link
Author

@2yunseong 2yunseong left a comment

Choose a reason for hiding this comment

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

파랑 안녕하세요! 1단계 리뷰 감사드려요!

남겨주신 사항은 말씀 주신 것 처럼 2단계 때 점진적으로 반영하도록 하겠습니다!!

감사합니다 😃

src/components/AddCardForm.tsx Show resolved Hide resolved
src/components/AddCardNumberInput.tsx Show resolved Hide resolved
});
};

return [value, onChange] as const;
Copy link
Author

Choose a reason for hiding this comment

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

useState처럼 비구조화 할당할 때 이름을 편리하게 붙이기 위해 배열을 리턴하였습니다!
안그러면 Hook을 사용할 때 마다 해당 이름을 다시 선언해 줘 불필요하게 코드가 길어진다고 생각합니다.
기능적인 차이는 어차피 배열 자체도 객체라고 생각했고, 여기서는 그렇게 큰 차이가 있지 않다고 생각해서 저는 배열로 선택하였습니다!
참고한 문서들 입니다.
https://tjinlag.medium.com/react-hooks-should-return-array-or-object-81b0a0fd4447
https://stackoverflow.com/questions/55214935/why-react-usestate-return-const-array

src/pages/AddCardPage.tsx Show resolved Hide resolved
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