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] 센트(김영우) 미션 제출합니다. #191

Merged
merged 42 commits into from
Apr 26, 2023

Conversation

kyw0716
Copy link
Member

@kyw0716 kyw0716 commented Apr 20, 2023

배포 링크 : https://kyw0716.github.io/react-payments

안녕하세요 헤인티!! 센트입니다😀

리뷰에 앞서 제가 작성한 컴포넌트들의 이해를 돕고자 각 컴포넌트의 역할에 대해 간략히 설명해드리겠습니다!

구조도

페이먼츠 프로그램 구조도


컴포넌트 설명

CardNumberInput

image

4자리 입력값을 받아오는 input 태그 4개로 이루어져 총 16자리의 카드 번호를 입력 받는 컴포넌트


ExpirationDateInput

image

2자리 입력값을 받아오는 input 태그 2개로 이루어져 총 4자리의 만료일을 입력 받는 컴포넌트


Input

중복되는 input 태그의 스타일을 styled-components를 사용해 다른 컴포넌트에서 활용할 수 있게 한 컴포넌트


InputWrapper

Input 컴포넌트를 가지고 있는 회색 박스 컴포넌트


OwnerNameInput

image
사용자의 이름을 영문으로 최대 30자 입력 받는 컴포넌트


PasswordInput

image
카드 비밀번호 앞자리 2개를 입력 받는 컴포넌트


SecurityCodeInput

image
보안코드 3자리를 입력 받는 컴포넌트


AddNewCardForm

image
모든 입력 값들을 객체로 묶어 localStorage에 등록하는 컴포넌트


CardViewer

image
넘겨 받은 카드 정보를 통해 카드 형태를 렌더링 해주는 컴포넌트


궁금한점

이번 미션을 진행하면서 저는 다음과 같은 궁금점이 생기게 되었습니다

- 메인 페이지에서 카드 등록 페이지로 넘어갔을 때 첫 번째 입력 받는 컴포넌트에 focus하는 방법

이번 미션처럼 input 태그가 많을 때 모바일 환경에서 input 태그를 직접 클릭해 입력하는 것이 정말 번거롭다는 느낌이 들어 각 입력값을 모두 입력하면 다음 input으로 focus가 넘어가도록 코드를 짜보았습니다. 이때 처음 화면이 렌더링 될 시 가장 첫번째로 입력할 input 태그에 자동으로 focus가 되도록 하고 싶었는데 input 태그의 autofocus 어트리뷰트를 이용하는 방법, React의 useRef를 통해 input에 ref를 전달해 이를 활용하는 방법 등을 시도해 보았지만 ios 환경에서는 focus가 아예 되지를 않고, 안드로이드 환경에서는 focus는 되지만 가상 키보드가 열리지 않았습니다. 혹시 이 문제를 헤인티는 어떻게 해결 하시나요?!


- 입력 완료시 다음 input으로 focus가 넘어가도록 하는 방법

지금은 각 input에 대해 모두 ref를 생성하고, 이를 활용해 이전 input에서 입력이 완료되면 다음 input의 ref의 focus 메서드를 호출하는 방식으로 코드를 작성했습니다. 하지만 이렇게 작성하다보니 코드의 가독성이 정말 떨어져 이 방식 이외에 다른 방식이 있는지 찾아보았지만 마땅한 방법을 찾아내지 못했습니다. 혹시 헤인티는 이럴 때 어떤 방식을 사용하시나요?!

kyw0716 and others added 28 commits April 18, 2023 14:43

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>
Co-authored-by: HyeryongChoi <chex1004@gmail.com>
Co-authored-by: HyeryongChoi <chex1004@gmail.com>
Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>

Co-authored-by: HyeryongChoi <chex1004@gmail.com>
Co-authored-by: HyeryongChoi <chex1004@gmail.com>
@kyw0716 kyw0716 changed the base branch from main to kyw0716 April 20, 2023 08:22
@HyeonaKwon HyeonaKwon self-requested a review April 21, 2023 01:15
@HyeonaKwon HyeonaKwon self-assigned this Apr 21, 2023
Copy link

@HyeonaKwon HyeonaKwon left a comment

Choose a reason for hiding this comment

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

안녕하세요 센트! 이번 미션의 리뷰어인 헤인티입니다 :) 만나서 반갑습니다!

빠른 시간내에 UI 와 로직, 그리고 스토리북까지 구현하셨다니 대단하네요! ㅎㅎ 컴포넌트도 역할에 맞게 잘 나눠져있는 것 같습니다.

하나 아쉬운거는 state 를 custom hook 으로 관리해보면서 더 다양한 시도를 해봤으면 좋았겠단 생각을 했습니다! 그리고 input 의 기본 속성을 props 로 받아서 사용처에서 다양한 속성들을 사용해볼 수 있었을 것 같아요. 예를 들면 type 이나 checked, disabled, 더 나아가서는 접근성 관련 속성들까지요!

이외에는 코멘트 확인 부탁드립니다! 미션 진행하느라 고생 많으셨어요!

질문 답변

메인 페이지에서 카드 등록 페이지로 넘어갔을 때 첫 번째 입력 받는 컴포넌트에 focus하는 방법

이거는 safari 정책이라 어쩔 수 없습니다 😭 유저의 인터렉션 없이 element 를 컨트롤하지 말자! 라는 기조라서 video 나 audio 도 자동재생할 수 없는 경우도 있습니다.

package.json Outdated
Comment on lines 10 to 15
"@types/jest": "^27.5.2",
"@types/node": "^16.18.23",
"@types/react": "^18.0.37",
"@types/react-dom": "^18.0.11",
"@types/react-router-dom": "^5.3.3",
"@types/styled-components": "^5.1.26",

Choose a reason for hiding this comment

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

types 패키지들은 devDependencies 로 설치해볼까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

넵! 반영 완료했습니다😀

수정한 커밋: df8ead0


function App() {
return (
<BrowserRouter basename={process.env.PUBLIC_URL}>

Choose a reason for hiding this comment

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

BrowserRouter 외에 다른 router 들도 있는데 각각의 차이점이 뭘까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • BrowserRouter
    • 브라우저의 주소 바 속의 현재 위치를 저장한다.
    • 브라우저의 내장 history stack을 통해 페이지를 이동시킨다.
  • HashRouter
    • 서버에 URL을 보내지 말아야하거나 보낼 수 없을 때 사용된다.
    • 현재 위치를 hash에 저장해 서버에 이를 보내지 않는다.
  • MemoryRouter
    • 위치를 배열에 저장한다.
    • 브라우저의 history stack과 같은 외부 자원에 묶여있지 않다.
    • history stack을 완벽히 제어할 수 있다.
  • NativeRouter:
    • React Native 앱에서 사용하는 것이 권장된다.

한번도 router의 종류에 대해 고민해본 적이 없어서 이번 기회에 한번 찾아보았습니다!! BrowserRouter외에도 많은 router들이 있었네요😀😀


참고한 자료

Choose a reason for hiding this comment

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

ㅎㅎ 좋습니다! 앞으로 개발할 때 어떤 상황에 어떤 라우터를 선택할지에 도움이 되었으면 좋겠네요!

Comment on lines 37 to 40
const monthInputRef = useRef<HTMLInputElement>(null);
const ownerNameInputRef = useRef<HTMLInputElement>(null);
const securityCodeInputRef = useRef<HTMLInputElement>(null);
const passwordInputRef = useRef<HTMLInputElement>(null);

Choose a reason for hiding this comment

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

forwardRef 를 이용해보셔도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

forwardRef라는 것이 있는지도 몰랐네요😭😭 남겨주신 링크 참고해서 반영해보았습니다!!

수정한 커밋: 17d1c69

Comment on lines 42 to 56
const moveFocusToExpirationDate = () => {
monthInputRef.current?.focus();
};

const moveFocusToOwnerName = () => {
ownerNameInputRef.current?.focus();
};

const moveFocusToSecurityCode = () => {
securityCodeInputRef.current?.focus();
};

const moveFocusToPassword = () => {
passwordInputRef.current?.focus();
};

Choose a reason for hiding this comment

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

입력 완료시 다음 input으로 focus가 넘어가도록 하는 방법

질문 내용이 여기인 것 같아 여기에 답변 남길게요!
센트가 하신 방법대로 해도 되고, 저라면 dataset 이용해서 다음 포커싱할 element 정보를 두고 조건 만족했을 때 dataset 에서 꺼내서 focus 할 것 같아요!
ref 가 이 포커싱 기능때문에 사용되는거라면 다 없애고 element id 를 dataset 에 저장해 getElementById 를 할 수도 있고, ref 를 사용한다면 refs[] 에 ref 들을 모아두고 index 를 dataset 에 저장할 수도 있구요!

센트처럼 각 함수 만들어도 좋은데요! 함수 갯수도 많아지고 props 로 왔다갔다 하니까 피로함도 있을 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

useFocus라는 커스텀 훅을 만들어서 이야기 해주신 내용 반영해보았습니다!!

수정한 커밋: 17d1c69
(커밋하는걸 까먹고 있다가 한꺼번에 해버려서 변경사항이 좀 많습니다😅)

Comment on lines 67 to 73
cardDataService.addNewCard({
cardNumber,
expirationDate,
ownerName,
securityCode,
password,
});

Choose a reason for hiding this comment

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

controlled component 와 uncontrolled component 중 제어컴포넌트를 사용하신 이유가 있을까요?

비제어를 썼다면 form 의 submit 이벤트의 event 객체에서 각 데이터를 꺼내서 활용할 수도 있고, state 를 따로 관리할 필요가 없어 AddNewCardForm 에서 state 를 다 관리해줄 필요도 없을 것 같은데요, 제어 컴포넌트로 관리하신 이유가 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

입력값이 변할 때마다 유저에게 보여주는 인터렉션을 구현하고 싶어 제어 컴포넌트로 구현해보았습니다!

ex) 입력값이 변할 때마다 에러 메세지를 변경해주기, 다음 입력 창으로 focus이동해주기

Choose a reason for hiding this comment

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

아하!

}

export function InputWrapper({ children, width }: InputWrapperProps) {
return <Style.Wrapper style={{ width: width }}>{children}</Style.Wrapper>;

Choose a reason for hiding this comment

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

Suggested change
return <Style.Wrapper style={{ width: width }}>{children}</Style.Wrapper>;
return <Style.Wrapper style={{ width }}>{children}</Style.Wrapper>;

Copy link
Member Author

Choose a reason for hiding this comment

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

수정 완료했습니다!!

수정한 커밋: 920a186

);
})}
</InputWrapper>
</>

Choose a reason for hiding this comment

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

여기도 input 의 타입을 바꿔보면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

타입 수정 완료했습니다!!

Comment on lines 19 to 21
useEffect(() => {
if (securityCode.length === 3) moveFocusToPassword();
}, [securityCode]);

Choose a reason for hiding this comment

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

useEffect 를 render 상단에 모아두면 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

수정 완료했습니다!!

alert('유효한 보안 코드가 아닙니다.');
}

setSecurityCode(e.target.value.toUpperCase());

Choose a reason for hiding this comment

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

toUpperCase 는 왜 하는거예요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

이전 컴포넌트를 복사 붙여넣기 해서 첫 시작을 하는 바람에 이상한 코드가 추가돼버렸네요 ㅠㅠ 수정했습니다!

JSON.stringify([card, ...storedCardList])
);
},
};

Choose a reason for hiding this comment

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

어차피 state 에 있는 카드번호에 대한 로직이니 hook 으로 작성했어도 됐을 것 같네요!

Copy link
Member Author

Choose a reason for hiding this comment

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

아직 훅 사용이 익숙지 않아서 훅으로 분리할 생각을 못했었네요! 이야기 해주신 내용 반영해보았습니다!!

수정한 커밋: 0dc33e3

Comment on lines +7 to +19
const inputElement = inputRefs.current[index];

if (index >= focusTargetCount && callback) {
callback();
return;
}

if (index < 0 && callback) {
callback();
return;
}

if (inputElement === undefined) return;

Choose a reason for hiding this comment

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

inputRefs.current 먼저 확인해보는게 좋을 것 같습니다!

Suggested change
const inputElement = inputRefs.current[index];
if (index >= focusTargetCount && callback) {
callback();
return;
}
if (index < 0 && callback) {
callback();
return;
}
if (inputElement === undefined) return;
if (!inputRefs.current) return;
const inputElement = inputRefs.current[index];
if (index >= focusTargetCount && callback) {
callback();
return;
}
if (index < 0 && callback) {
callback();
return;
}

Comment on lines +30 to +47
const {
inputRefs: cardNumberInputRefs,
focusInputByIndex: focusCardNumberInputByIndex,
} = useFocus(4);

const {
inputRefs: expirationDateInputRefs,
focusInputByIndex: focusExpirationDateInputByIndex,
} = useFocus(2);

const { inputRefs: ownerNameInputRefs } = useFocus(1);

const { inputRefs: securityCodeInputRefs } = useFocus(1);

const {
inputRefs: passwordInputRefs,
focusInputByIndex: focusPasswordInputByIndex,
} = useFocus(2);

Choose a reason for hiding this comment

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

useFocus 를 따로따로 사용하니까 이름 카드번호 비밀번호 등등의 다음 인풋으로 이동이 힘들 것 같네요!

Copy link

@HyeonaKwon HyeonaKwon left a comment

Choose a reason for hiding this comment

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

안녕하세요 센트!
useFocus 적용하시면서 각 인풋에 focus 관련된 함수를 넘겨주고 계신데 굳이 props 이름이 다 달라야하나 싶습니다! 그냥 다음 인풋을 선택하는 함수다 라는 것만 인지시키면 될 것 같아서 Input 공통 컴포넌트에 해당 로직을 넣어도 될 것 같구요. 한번 생각해봐주세요!

이번 미션은 여기서 머지하도록 하겠습니다~ 고생하셨어요 ㅎㅎ

@HyeonaKwon HyeonaKwon merged commit 73a05e5 into woowacourse:kyw0716 Apr 26, 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