-
Notifications
You must be signed in to change notification settings - Fork 165
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] 아커(전증훈) 미션 제출합니다. #253
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.
안녕하세요 아커~!
저도 다시 만나서 영광입니다
전반적으로 훅이 필요에 비해 많이 만들어진 것 같아요. 훅도 함수와 같이 생각해주시면 됩니당. 어떤 코드를 함수로 추상화했을때 얻는 이득이 크지 않다면 굳이 함수로 분리할 필요가 없을 것 같아요
커스텀훅을 만드는 기준을 재고해봐도 좋을 것 같습니다~!
커스텀 훅 분리
로직을 훅으로 분리하는 것에 대한 기준이 아직 모호합니다.
커스텀 훅은 리액트 라이프 사이클이 가미된 유틸성 함수를 훅으로 분리한다는 정도로 알고 있습니다.
그렇게 분리하다 보니, 이게 과연 다른 컴포넌트에서도 사용될 수 있을까? 싶은 훅들이 종종 나왔습니다. 재사용하기 어려울 거라 생각한 이유는 파라미터로 어떤 값이 들어올 지 모르기 때문에 타입 관련해서 어려웠던 것같습니다.
예를 들어 CardInfo에는 여러 가지 정보들이 담겨 있습니다. 카드 정보, 만요일, 이름 ... 등등 다양한 타입의 값들이 들어있습니다. 이러한 모든 타입에 대응하는 훅을 만들었는데 (useInputBox) 결국 해당 프로젝트에서만 사용될 수 있는 도메인 유틸같다는 생각을 했습니다.
이런 식으로 분리해도 괜찮을까요?
아커는 어떤 로직을 함수로 분리하는 기준이 어떻게 되나요?
거기에는 재사용만이 기준으로 존재하지는 않을 것 같아요.
레이싱카에서 사용했던 게임을 진행하는 함수는 레이싱카 프로그램에서 밖에 사용하지 못할 거예요.
반면에 storage API를 추상화한 함수는 여러 프로젝트에서 사용할 수 있을테구요.
훅도 마찬가지라고 생각해주셔도 좋을 것 같아요. 리액트 함수컴포넌트에서 사용하는 보다 좁은 범위의 함수정도면 깔끔하게 정의해볼 수 있을까싶네요
Context API
카드 정보, 카드 정보 리스트를 전역적으로 관리하고 있습니다. 이렇게 사용해도 될지 의문입니다.
거의 모든 state가 최상단에서 관리되고 있으니 매번 리렌더링이 일어나서 효율성이 떨어질 것같습니다.
그렇지만 아직 리액트를 배우는 입장이라 생각하기 때문에, 리액트에 익숙해지기 전에 효율을 따지지 말자라고 생각하고 있지만 마음에 걸리네요.
input이 많으면 성능이 생각보다 빠르게 안좋아져요. 그래서 제어/비제어 개념에 대해서 집중도 있게 학습해보셔도 좋을 것 같습니다.
너무 복잡한 컴포넌트
한 컴포넌트 안에서 너무 많은 훅들을 요청해서 사용합니다. 컴포넌트를 분리해야 하는 신호일까요?
CardAliasPage 라는 카드 별칭 페이지에서 5개의 커스텀 훅을 사용합니다. 그 중 2개의 훅은 타이틀 메시지에, 그리고 2개의 훅은 버튼 컨테이너에서만 사용되는데, 타이틀과 버튼 컨테이너를 분리하면 될까요?
위에서 말씀드렸듯 과도하게 훅을 추상화해서 사용하구 있어요. 제 입장에서는 그렇게 복잡해보이는 페이지는 아닌것같긴한데, 컴포넌트 분리의 기준을 한 번 잡아보면 어떨까 싶네요. '훅이 많다'는 것도 지표가 될 수 있지만 항상 이 기준이 부합하는 것은 아닐 것 같아요. 아커의 컴포넌트 분리 기준을 한 번 정해보는 시간을 가져보셔도 좋겠습니다~!
모바일 환경에서의 뷰포트
제 환경은 아이폰13프로입니다. 배포한 사이트를 들어가봤더니, 화면이 100vh가 아닌, 120vh 정도 되는 것같더라구요 🥲
검색을 해보니 주소창 영역과 하단의 툴바 때문에 그렇다는데, 현업에서는 이런 다양한 환경에 대한 CSS 대응을 어떻게 하나요?
viewport 메타태그에 준 값이랑 연관이 있을 것도 같아요~! safe area 도 여기서 챙겨보시면 좋을 것 같네요
Input 컴포넌트의 type
현재 모든 Input 컴포넌트의 type은 text입니다. 이렇게 지정하니 모바일에서 사용할 때, 숫자를 입력해야 하는 칸에 text를 입력하는 키보드가 나와서 매우 불편하더라구요. 그래서 number로 바꿔봤는데, 그렇게 하면 잘못된 입력을 넣었을 때 오류 메시지를 보여줄 수 없게 됩니다. 애초에 잘못된 입력이 들어가지 않아서... (웹 환경에서 입니다.)
오류를 막는 것은 좋지만, 입력이 되지 않는데 피드백 조차 없는 것은 불편한 UX라고 생각됩니다.
파노의 의견이 궁금합니다!
inputmode라는 속성에 대해 알아보셔도 좋을 것 같아요! 가상키보드가 어떻게 떠야하는지 지정해줄 수 있을 것으로 보여요
중복된 컴포넌트의 키
현재 Card나 Input과 같이 컴포넌트들이 중복되어 여러 번 렌더링되는 경우가 있습니다. 이럴 경우 보통 어떤 값을 key로 넘겨주시나요?
상황에 따라 다를 것 같습니다. 요소의 순서가 키값이 될수도 있을테구요. 데이터 객체에 고유한 id값이 있다면 그걸 사용하기도 해요. 유니크한 키값을 줄 수 있도록 노력해보시면 좋을 것 같아요!
카드사 선택 모달의 백드롭
백드롭을 클릭할 때, 어떤 상황이 더 어울릴지 고민입니다.
카드사 정보는 필수 값이니 클릭을 무시한다.(현재 상황)
카드사 정보는 필수 값이니 홈 페이지로 이동한다.
모달을 닫고 사용자가 칸에 입력을 하려고 하면, 다시 모달을 띄워 카드사 정보를 받게 한다.
카드사 정보는 마지막에 입력 받거나 중간에 사용자가 입력 받을 수 있게 모달을 닫을 수 있게 한다.
UX는 논리가 있다면 일단 오케이입니다. 고것이 일반적으로 유저에게 어필되는 UX인지는 부차적인 문제같아요. 계속해서 가다듬는 과정을 반복해보시면 요런 UX설계도 아커의 장점이 될 수 있을 것으로 생각합니다. 저라면 자유도를 좀 더 생각해서 스킵이 가능하게 하되 제출만 막는 경험을 고려해볼 것 같아요
코멘트가 너무 많아서 여기서 일단 짜르고 스텝3에서 추가적으로 같이 봐요!
화이팅입니당
src/GlobalStyles.ts
Outdated
@@ -9,7 +10,7 @@ const GlobalStyles = createGlobalStyle` | |||
} | |||
|
|||
body { | |||
width: 100vw; | |||
width: 100%; |
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.
100vw 에서 100%로 변경한 이유는 어떤 것인가요?
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.
IOS환경 레이아웃 수정하면서 이것 저것 만져보다가 실수로 변경됐습니다! 다시 100vw로 수정했습니다.
|
||
import * as styled from './BottomSheet.styled'; | ||
|
||
const BottomSheet = ({ CardCompanyContents }: { CardCompanyContents: ReactNode }) => { |
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.
CardCompanyContents를 바텀싯이 알아야할것같진 않아서 children정도여도 좋을 것 같아요~
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.
말씀하신 대로 수정해서 바텀싯의 유틸성이 더욱 올라갔습니다!
src/components/Card/Card.styled.ts
Outdated
|
||
background-color: ${props => props.bgColor ?? COLOR.DEFAULT}; | ||
background-color: ${props => props.theme ?? COLOR.DEFAULT}; |
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.
테마여서 프리셋으로 지정된 문자열으로 예상했는데 컬러값이네요! backgroundColor로 명확하게 변경해줘도 좋을 것 같아요
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.
피드백 주신 대로 수정했습니다!
src/components/Card/Card.styled.ts
Outdated
box-shadow: 3px 3px 5px rgba(0, 0, 0, 0.25); | ||
border-radius: 5px; | ||
|
||
color: ${COLOR.WHITE}; | ||
color: ${props => (props.theme === '#FFE600' ? COLOR.BLACK : COLOR.WHITE)}; |
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.
폰트컬러가 배경색에 따라 변경되는데 요거 계산식이 있을지 한 번 찾아보면 어떨까요?
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.
이 부분에 대해 찾아보다 mix blend mode 라는 걸 찾아서 적용해봤습니다. 그러나 특정 배경색에 원하는 폰트 색을 입히고 싶어서 해당 css 속성은 사용하지 않았습니다.
따로 colorGenerator라는 함수에서 배경 색을 넣으면 폰트 색을 반환하는 함수를 만들어서 재사용했습니다.
src/components/Card/Card.tsx
Outdated
@@ -4,27 +4,28 @@ import * as styled from './Card.styled'; | |||
|
|||
export interface CardProps { | |||
cardInfo: CardInfo; |
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.
Info는 붙어서 어떤 의미가 추가되지 않는 불용어이기 때문에 요기 그냥 Card로 명명해주면 어떨까 싶네요!
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.
피드백 주신 대로 수정했습니다!
|
||
import * as styled from './CardRegisterButtonContainer.styled'; | ||
|
||
const CardRegisterButtonContainer = () => { |
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.
컨테이너 컴포넌트가 이미 내부에 요소를 갖고있는것같은데 CardRegisterButton으로 명명되는게 자연스러울 것 같아요
const CardRegisterForm = () => { | ||
return ( | ||
<styled.CardRegisterForm> | ||
<CardNumberInputBox /> | ||
<ExpirationDateInputBox /> | ||
<OwnerNameInputBox /> | ||
<SecurityCodeInputBox /> | ||
<PasswordInputBox /> | ||
<CardInfoSubmitButtonContainer /> | ||
</styled.CardRegisterForm> | ||
); | ||
}; |
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.
깔끔하네요 👍
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.
요소타입이 form 이었다면 submit핸들러함수가 여기서 사용되어서 로직이 위계적으로도 적절한 위치에 있었을것같아요
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.
앗 칭찬 감사합니다 !!! ㅎㅎㅎ
핸들러 함수는 해당 컴포넌트에서 관리하도록 수정했습니다!
@@ -0,0 +1,14 @@ | |||
import styled from 'styled-components'; | |||
|
|||
export const CardRegisterForm = styled.div` |
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.
form이 아닌데 form으로 명명됐어요. 현재 브라우저레벨에서 지원해주는 자동완성기능같은것들이 동작하지 않아요
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.
https://wainaat.github.io/react-payments 참새가 한 거는 자동완성이 잘되더라구요. 참고해봐도 좋을 것 같아요
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.
form으로 수정했습니다!
form을 submit하면 url이 바뀌는 문제 때문에 div로 명명했는데, 이것 때문에 자동완성이 안된거였군요 …
src/components/Header/Header.tsx
Outdated
const Header = () => { | ||
const { pathname } = useLocation(); | ||
const navigation = useNavigate(); | ||
|
||
const onClickBackward = () => { | ||
navigation('/'); | ||
}; | ||
const { isOnRegisterPage, pageTitle } = useHeader(); | ||
const { navigationTo } = useNavigationTo(PATHNAME.HOME); | ||
|
||
return ( | ||
<styled.Header> | ||
{pathname !== PATHNAME.HOME && <styled.BackwardButton onClick={onClickBackward}>{'<'}</styled.BackwardButton>} | ||
<styled.HeaderTitle onClick={onClickBackward}>{PAGE_TITLE[pathname as keyof PageTitle]}</styled.HeaderTitle> | ||
{isOnRegisterPage && ( | ||
<styled.BackwardButton onClick={navigationTo}>{`<`}</styled.BackwardButton> | ||
)} | ||
<styled.HeaderTitle onClick={navigationTo}>{pageTitle}</styled.HeaderTitle> | ||
</styled.Header> | ||
); | ||
}; |
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.
페이지 타이틀과 뒤로가기버튼을 별도 모듈에서 선언하고 있는데 페이지컴포넌트 모듈에서 관리하면 보다 연관깊은코드가 가까이에 있게될것같아요. PageLayout 컴포넌트와같이 페이지 컴포넌트가 공통적으로 사용하는 컴포넌트에 헤더컴포넌트를 포함시켜서 페이지컴포넌트에서 prop으로 넘겨주도록 변경해보면 어떨까요?
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.
페이지 레이아웃 컴포넌트를 만들어서 내부에서 헤더 컴포넌트와 children
으로 전달받은 여러 페이지들을 렌더링하는 구조로 수정했습니다.
그러나 말씀하신 부분에서 이해가 잘 되지 않는 부분이 있습니다.
prop으로 넘겨 주어야 하는 것이 pageTitle
과 같은 변수인지, 혹은 HeaderTitle
과 같은 ReactNode
인지 헷갈립니다…
일단 HeaderTitle
과 같은 컴포넌트는 당연히 Header
컴포넌트에서 관리 되어야 한다고 생각하기 때문에 전자인 pageTitle
과 같은 변수를 넘겨주는 방식으로 수정했습니다.
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.
고것은 얼마나 커스텀이 가능하게 만드느냐의 차이일 것 같은데요. 디자인이 항상 같은 논리로, 동일한 형태로 적용되는 것은 아니라.. 가능한 커스텀에 대한 가능성을 고려하게 되는것같아요
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.
string, reactNode둘 다 받게해도 상관없을것같아요
src/hooks/useTitle.ts
Outdated
export const useTitle = (titleValue: string) => { | ||
const [title, setTitle] = useState(''); | ||
|
||
useEffect(() => { | ||
setTitle(titleValue); | ||
}, []); | ||
|
||
return title; | ||
}; |
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.
-
요런 단순 setState는 커스텀훅으로 분리하지 않아도 복잡도가 큰 차이가 없는 것 같아요
export const useTitle = (titleValue: string) => { | |
const [title, setTitle] = useState(''); | |
useEffect(() => { | |
setTitle(titleValue); | |
}, []); | |
return title; | |
}; | |
export const useTitle = (titleValue: string) => { | |
const [title, ] = useState(titleValue); | |
return title | |
}; |
요렇게 해도될것같은데 useEffect에서 하는 이유는 어떤 것인가요?
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.
useState를 사용할 때, 꼭 setState도 함께 사용해야 하는 줄 알고 있었습니다…! 이런 방법도 있다니 신기하군요!
생각해보니 해당 페이지의 타이틀은 현재로서는 절대 바뀔 일이 없는 타이틀인 것같아 상태로 관리하지 않도록 수정했습니다!
안녕하세요 파노! 리팩터링 목록
질문
|
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.
코멘트 잘 반영해주셔서 감사합니다! 컴포넌트 구성에 큰 이견은 없어요.
궁금한 점은 아커가 찾아보도록 요청을 드리고 있는데 요게 요청이 많아서 부담이 되는지, 혹여 추상적이진 않은지 궁금하네요~!
스텝3에서 뵙도록 할게요
배포 페이지
안녕하세요 파노! 😊
step2로 다시 만나뵙게 되어 영광이고 기쁩니다!
이번 리뷰도 잘 부탁드립니다!
학습한 부분
이번 미션에서 진행한 부분은 다음과 같습니다.
앱 구조
진행하면서 어려웠던 부분
커스텀 훅 분리
로직을 훅으로 분리하는 것에 대한 기준이 아직 모호합니다.
커스텀 훅은 리액트 라이프 사이클이 가미된 유틸성 함수를 훅으로 분리한다는 정도로 알고 있습니다.
그렇게 분리하다 보니, 이게 과연 다른 컴포넌트에서도 사용될 수 있을까? 싶은 훅들이 종종 나왔습니다. 재사용하기 어려울 거라 생각한 이유는 파라미터로 어떤 값이 들어올 지 모르기 때문에 타입 관련해서 어려웠던 것같습니다.
예를 들어 CardInfo에는 여러 가지 정보들이 담겨 있습니다. 카드 정보, 만요일, 이름 ... 등등 다양한 타입의 값들이 들어있습니다. 이러한 모든 타입에 대응하는 훅을 만들었는데 (useInputBox) 결국 해당 프로젝트에서만 사용될 수 있는 도메인 유틸같다는 생각을 했습니다.
이런 식으로 분리해도 괜찮을까요? ...
Context API
카드 정보, 카드 정보 리스트를 전역적으로 관리하고 있습니다. 이렇게 사용해도 될지 의문입니다.
거의 모든 state가 최상단에서 관리되고 있으니 매번 리렌더링이 일어나서 효율성이 떨어질 것같습니다.
그렇지만 아직 리액트를 배우는 입장이라 생각하기 때문에, 리액트에 익숙해지기 전에 효율을 따지지 말자라고 생각하고 있지만 마음에 걸리네요.
너무 복잡한 컴포넌트
한 컴포넌트 안에서 너무 많은 훅들을 요청해서 사용합니다. 컴포넌트를 분리해야 하는 신호일까요?
CardAliasPage 라는 카드 별칭 페이지에서 5개의 커스텀 훅을 사용합니다. 그 중 2개의 훅은 타이틀 메시지에, 그리고 2개의 훅은 버튼 컨테이너에서만 사용되는데, 타이틀과 버튼 컨테이너를 분리하면 될까요?
모바일 환경에서의 뷰포트
제 환경은 아이폰13프로입니다. 배포한 사이트를 들어가봤더니, 화면이 100vh가 아닌, 120vh 정도 되는 것같더라구요 🥲
검색을 해보니 주소창 영역과 하단의 툴바 때문에 그렇다는데, 현업에서는 이런 다양한 환경에 대한 CSS 대응을 어떻게 하나요?
Input 컴포넌트의 type
현재 모든 Input 컴포넌트의 type은 text입니다. 이렇게 지정하니 모바일에서 사용할 때, 숫자를 입력해야 하는 칸에 text를 입력하는 키보드가 나와서 매우 불편하더라구요. 그래서 number로 바꿔봤는데, 그렇게 하면 잘못된 입력을 넣었을 때 오류 메시지를 보여줄 수 없게 됩니다. 애초에 잘못된 입력이 들어가지 않아서... (웹 환경에서 입니다.)
오류를 막는 것은 좋지만, 입력이 되지 않는데 피드백 조차 없는 것은 불편한 UX라고 생각됩니다.
파노의 의견이 궁금합니다!
중복된 컴포넌트의 키
현재 Card나 Input과 같이 컴포넌트들이 중복되어 여러 번 렌더링되는 경우가 있습니다. 이럴 경우 보통 어떤 값을 key로 넘겨주시나요?
카드사 선택 모달의 백드롭
백드롭을 클릭할 때, 어떤 상황이 더 어울릴지 고민입니다.