Skip to content

[1단계 - 페이먼츠] 무비(변영화) 미션 제출합니다.#98

Merged
EungyuCho merged 54 commits intowoowacourse:byhhh2from
byhhh2:byhhh2
May 4, 2022
Merged

[1단계 - 페이먼츠] 무비(변영화) 미션 제출합니다.#98
EungyuCho merged 54 commits intowoowacourse:byhhh2from
byhhh2:byhhh2

Conversation

@byhhh2
Copy link

@byhhh2 byhhh2 commented Apr 29, 2022

안녕하세요 아사님! 로또 미션 이후로 다시 뵙게 됐네요. 너무너무 반갑습니다 🥰
이번 미션은 리액트를 사용해보았고, CDD를 중점으로 미션을 진행했습니다.
잘 부탁드릴게요 ~!! 😆


페이먼츠 데모 페이지


미션 요구사항

작성 요구사항

  • 모바일 타겟의 웹 앱을 구현하며 컴포넌트가 가지는 의미와 편리한 모바일 UI/UX에 대해 고민해봅니다.
  • 다른 라이브러리나 프레임워크 없이 오로지 React만으로 상태를 관리하고 컴포넌트를 설계합니다.
  • Controlled & Uncontrolled Components에 입각하여 Form을 핸들링합니다.
  • 재사용 가능한 Component를 직접 작성하고 사용합니다.
  • React Hooks API를 활용합니다.
  • Storybook를 이용해서 상호 작용 테스트를 한다.

기능 요구사항 (직접 작성)

  • [공통] 사용자가 정보 입력시, 입력한 정보가 카드 이미지에 실시간으로 업데이트된다.
  • 카드사를 선택할 수 있다.
    • 카드 이미지 클릭 시, 카드사 선택 모달창이 화면에 보여진다.
    • 카드사를 선택하면 폼의 글씨색이 카드사 색으로 변경된다.
  • 카드 번호를 입력할 수 있다.
    • 앞의 8자리 숫자는 화면에 보여지고, 뒤의 8자리 숫자는 가려진다.
  • 만료일을 입력할 수 있다.
  • 카드 소유자 이름을 입력할 수 있다.
    • 카드 소유자 이름은 사용자 선택 하에 입력될 수 있다.
    • 이름은 30자 이내로 입력가능하다.
    • 사용자가 이름을 입력할 때 실시간으로 자릿수를 화면에 업데이트한다.
  • 보안 코드를 입력할 수 있다.
    • 보안 코드는 화면에서 가려진다.
  • 카드 비밀번호를 입력할 수 있다.
    • 카드 비밀번호는 앞의 2자리만 입력받는다.
    • 카드 비밀번호는 화면에서 가려진다.
  • 입력 폼에 필수 정보가 모두 입력된 경우, 다음 버튼이 화면에 표시된다.

미션에 대한 고민들

컴포넌트의 재활용성

컴포넌트의 재활용성을 집중적으로 생각하면서 미션을 수행했는데요!
어떤 부분을 어떻게 컴포넌트화 해야할 지에 대해 고민해본 것 같습니다.
첫번째 고민은 반복되는 input태그들을 어떻게 컴포넌트화할까에 대한 고민이였습니다.

  1. input태그만 컴포넌트화 한다.
  2. labelinput등의 태그를 묶어서 컴포넌트화 한다.

저는 2번 방식으로 개발을 진행했는데,

스크린샷 2022-05-01 오전 1 34 25

요론식으로 input 태그의 개수가 달라진다거나, 세부사항이 다르다거나 (어떤 경우에는 input value를 숨겨야한다.) 해서 개발할 때 고려사항이 많았고, 시간이 많이 걸렸습니다.

제가 1번 방식으로 하지 않은 이유는 input 태그만 하면 컴포넌트로 분리하는게 의미가 있을까? 그냥 <input /> 과 다른 점이 뭘까에 대한 의문때문이였습니다. 그런데 이런 문서를 참고하면 <input />만 존재하지만 스타일도 함께 묶어준다는 의미가 있다는 것을 알게 되었습니다.

하지만 1번 방식은 아직도 의문이 들긴 합니다. 이 부분에서는 아사님은 어떻게 생각하시나요?!


useState를 여러개 사용 vs 한번에 묶어서 사용

// 1. `useState`를 여러개 사용

const [cardCompany, setCardCompany] = useState('')
const [cardNumber, serCardNumber] = useState('')
const [expiryDate, setExpiryDate] = useState({ month: '', year: '' })

...
// 2. 한번에 묶어서 사용 

const initialCardInfo = {
  company: '',
  number: {
    first: '',
    second: '',
    third: '',
    fourth: '',
  },
  expiryDate: {
    month: '',
    year: '',
  },
  ...
};

const [cardInfo, setCardInfo] = useState(initialCardInfo) 

이 주제에 대해 페어와 토론을 진행했습니다.

1번 방식

  • 장점 : 갑을 업데이트 하기 위해 접근할 때 depth가 적다.
  • 단점 : useState()가 많아진다.

2번 방식

  • 장점 : 나중에 모든 값을 총합해서 넘겨주어야 할 때 편하다.
  • 단점 : 접근할 때 depth가 늘어난다.

결과적으로 2번 방식을 선택했습니다. 2번 방식의 장점이 크다고 생각했고, 1번 방식으로 하게 되면 input태그의 onChange에 달아줄 함수를 state에 따라 매번 바꿔줘야 해서 코드가 쓸데 없이 길어질 것이라고 생각했습니다. (page/cardAdd/index에서 handleChange함수)
이 부분에 대해서 아사님은 어떻게 생각하시나요?!


부모의 setState함수를 자식에게 직접적으로 props를 통해 넘겨주는 것은 옳은가?

// 1번
const Parent = () => {
	const [count, setCount] = useState(0);
	return <Children setCount={setCount} count={count}/>
}

const Children = ({count, setCount}) => {
	return <div onClick={()=> setCount(prev => prev + 1}>{count}</>
}

// 2번
const Parent = () => {
	const [count, setCount] = useState(0);
	const handleCount = () => setCount(prev => prev + 1);	

	return <Children handleCount={handleCount} count={count}/>
}

const Children = ({count, handleCount}) => {
	return <div onClick={handleCount}>{count}</>
}

1번 방식은 부모의 setCount함수를 그대로 넘겨주고 있고, 2번 방식은 handleCount로 감싸서 props로 넘겨주고 있습니다.
저는 사실 부모의 setState함수를 그래도 넘겨주는 것이 별 문제 없다고 생각했습니다. 그런데 페어가 자식에게 부모 state의 제어권을 직접적으로 주는 것은 조금 의문이 든다고 했습니다. 그래서 저희가 내린 해결책은 handleCount로 한번 감싸자!였습니다.

지나가던 코치님께 이에 대해 질문을 했었는데, handleCount로 감싸도 차이가 별 의미가 없다. 리고 말씀해주셨습니다.

저는 그래서 setCount를 직접적으로 넘기나, handleCount로 감싸고 넘기나 별 차이가 없다면, 자식에게 state를 바꾸는 어떠한 방식의 함수도 넘기지 않는게 좋은 걸까? 라는 의문이 들었습니다.

이에 대한 아사님의 생각은 어떤가요?




이번 미션도 잘 부탁드리고 긴글 읽어주셔서 감사합니다 😆~!
천천히 리뷰 달아주세요~!

byhhh2 and others added 30 commits April 26, 2022 16:06
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
byhhh2 and others added 10 commits April 28, 2022 21:56
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Co-authored-by: lokba <lokba@users.noreply.github.com>
Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

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

오랜만이에요 무비님! 반가워요 ㅎㅎ

전체적으로 코드 스타일도 일관된 상태라고 느껴졌고 페이지에서 컴포넌트의 위계도 잘 맞춰졌다고 생각했어요. 잘 구현해주셨습니다! 👏

미션에 대한 다양한 고민들도 자세하게 설명해주셨네요! 질문의 배경을 이해하는데 도움이 많이 됐어요 👍
우선 질문주신 부분들과 몇가지 코멘트를 코드블록에 코멘트로 드렸어요. 오후에 추가적으로 코멘트 달아드릴게요!.. 😇 😴

Etc

  • 스토리북에 카드 테마도 바꿀 수 있으면 좋겠어요! control 문서
  • 파비콘이 익숙한 그곳이네요!.. 😆

Comment on lines 169 to 178
<div className="flex-wrap">
{cardCompanyList.map(({ id, company, theme }) => (
<CardCompany
key={id}
company={company}
theme={theme}
onClickCompany={handleClickCompany}
/>
))}
</div>

Choose a reason for hiding this comment

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

부모의 setState함수를 자식에게 직접적으로 props를 통해 넘겨주는 것은 옳은가?

이 코멘트를 이야기 해볼 수 있을거같아요.
코치님이 생각하시는 명확한 정답은 유추되진 않지만 제가 생각하기에 옳은 방향성을 이야기해보려고해요.

우선 <CardCompany /> 컴포넌트의 속성으로 onClickCompany라는 prop이 있고 handleClickCompany를 함수로 넣어주셨어요.

다른 코드를 보지 않고 두가지 코드를 봤을 때 어떤 동작이 유추될까요?
저는 크게 얻을만한 정보가 없다고 생각해요. 회사 컴포넌트에 대해 클릭을 하면 무언가 변화가 일어나겠다는건 알 수 있겠는데 정확히 어떤 현상이 일어나는지에 대해서는 정보를 얻을 수 없어보여요.

직관적으로 화면에 필요한 정보와 인터렉션에 대한 결과를 유추할 수 있게 작성하는게 중요하다고 생각해요. 🤔
결론은 'setState보다는 직관적인 함수를 만들어 인자로 넣어주는게 좋을거같다!' 입니다~~
etc) 혹시나 추후에 정답을 알게되시면 저한테도 귀띔해주세요! 🤩

Copy link
Author

Choose a reason for hiding this comment

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

아하 제가 아사님의 comment를 제대로 이해한게 맞다면,
아사님은 setState를 넘겨주냐, 마냐에 초점을 두신게 아니라
'행동을 유추할 수 있게 setState를 넘겨주냐'에 초점을 두셨군요!

코치님께도 좀 더 자세히 여쭤본 후 귀띔해드리겠습니다 🤩

};

const CardAppPage = () => {
const [cardInfo, setCardInfo] = useState(initialCardInfo);

Choose a reason for hiding this comment

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

useState를 여러개 사용 vs 한번에 묶어서 사용

현재 요구사항에서는 1번과 2번 모두 크게 문제없는 방법이라고 생각해요.
하지만 상태가 추가적으로 늘어난다면 1번의 방향성이 더 유리하다고 생각이 드는데요~

모든 상태가 모여있다면 관리하기가 편하겠지만 상태가 많아질수록 전개연산자로 업데이트 할 속성이 많아지기 때문에 관리하기 힘들다고 생각해요. 오히려 상태를 분리해서 custom hook으로 필요한 상태와 함수들을 응집도있게 관리하는편이 장기적으로 유지보수에도 더 편할거같다고 생각이 들어요 👀

1번 방식으로 하게 되면 input태그의 onChange에 달아줄 함수를 state에 따라 매번 바꿔줘야 해서 코드가 쓸데 없이 길어질 것이라고 생각했습니다. (page/cardAdd/index에서 handleChange함수)

이 부분에 대한 대답이 됐을거같아요! 관리는 편하겠지만 불필요하게 전개연산자를 많이 사용하게 될수도있다!

물론 현재정도 사이즈의 미션의 상태는 2번도 좋은거같네요!~

Copy link
Author

Choose a reason for hiding this comment

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

맞아요! depth가 늘어난다는 말의 뜻이

전개연산자로 업데이트 할 속성이 많아지기 때문에 관리하기 힘들다고 생각해요.

딱 이 문장이였던 것 같아요!

현재 사이즈의 미션까지는 2번의 방식도 괜찮군요! 🤔
custom hook을 적용하는 것에 대해서도 많은 고민을 해볼게요!

Choose a reason for hiding this comment

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

여기있는 이벤트 핸들러는 익명함수로 만드는거보다 위에서 함수를 정의해서 사용하는걸 추천드릴게요!
렌더링시마다 inputInfList의 배열 개수만큼 익명함수를 생성하는 방식으로 렌더링될거에요 👀

Copy link
Author

Choose a reason for hiding this comment

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

리액트의 컴포넌트의 render 방식에 대해 잘 모르고 있었던 것 같아 조금 공부를 해봤는데요!!

함수형 컴포넌트는 함수로 만들어졌기 때문에 render를 위해 실행될 때마다 내부 함수가 새로 생성됩니다.

그러므로, <input />에게 inline으로 익명함수를 전달해주는 것은 <input />이 렌더링될 때마다 익명함수가 생성이 됩니다. 즉, inputInfoList의 요소 개수만큼 생성이 되겠죠!

아사님이

익명함수로 만드는거보다 위에서 함수를 정의해서 사용하는걸 추천드릴게요!

이렇게 말씀해주신 이유는 inputInfoList가 렌더될 때마다 함수가 생성되는 것보다 FormInput이 렌더될 때마다 함수가 생성되는 게 더 낫다는 뜻이겠죠?!

그런데! 또 FormInput도 부모에게 받은 props에 따라 재렌더가 되기 때문에 useCallback으로 감싸주는 것이 더 나을 것이라고 생각했어요!!! 이런 이유로 useCallback을 사용하도록 수정해주었습니다.

commit : 2473aad

Choose a reason for hiding this comment

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

멋지네요 무비!
제가 명확히 정답을 안드렸는데도 제가 맞다고 생각한 정답을 도출해내셨어요 💯

Comment on lines 61 to 68

Choose a reason for hiding this comment

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

input 태그만 하면 컴포넌트로 분리하는게 의미가 있을까? 그냥 과 다른 점이 뭘까에 대한 의문때문이였습니다. 그런데 이런 문서를 참고하면 만 존재하지만 스타일도 함께 묶어준다는 의미가 있다는 것을 알게 되었습니다.

분리할 필요성이 없다면 분리하지 않아도 좋다고 생각해요. 불필요하게 공통적인 스타일링이 많이 들어가거나 더 선언적으로 해결해보고 싶은 부분이 있다면 그때가서 분리해도 좋다고 생각해요.

제가 작성한다면 현재단계에서는 라벨, input을 합쳐서 컴포넌트를 만드는걸 선택할거같네요 🙂

Copy link
Author

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.

propTypes는 어떤 문제를 해결할 수 있을까요?
어떤 장점이 있는지 궁금해요

Copy link
Author

Choose a reason for hiding this comment

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

propTypes의 장점 ✨

  • typescript를 사용하지 않고도, react 내장 기능을 통해 타입 검사를 수행하여 버그를 예방할 수 있다.
  • 타입 정의만으로도 문서화를 할 수 있다.
    • 코드를 읽는 사람이 로직을 이해하지 않고도 props의 정보를 유추할 수 있다.
    • propsType에 주석을 추가함으로써 storybook의 docs description을 추가할 수 있다.
  • isRequired 를 통해 꼭 보내야하는 값을 지정할 수 있어서 오류를 예방할 수 있다.

등이 있을 것 같습니다~!

Choose a reason for hiding this comment

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

useModal 형태의 custom hook으로 분리해보는건 어떨까요?
자주 쓰이는 속성이다보니 바로 사용할 수 있게 custom hook으로 만들어두면 편할거같아요!!

Comment on lines 95 to 98

Choose a reason for hiding this comment

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

참고로 toggle만 한다면 reducer를 통해 간단하게 작성할 수 있어요!

Suggested change
const handleModal = useCallback(() => {
setModalVisible((prevModalVisible) => !prevModalVisible);
}, []);
const [modalVisible, handleModal] = useReducer((visible) => !visible, false);

Copy link
Author

@byhhh2 byhhh2 May 2, 2022

Choose a reason for hiding this comment

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

헉,,useReducer라는 좋은 hook이 존재했군요!! 리액트에 익숙하지 못해서 그런지 전혀 모르고 있었네요 ㅎㅎ.
modal관련해서는 custom hook을 만들기보다는 이렇게 간단하게 useReducer를 쓰는게 좋은 것 같아요!

아사님 덕분에 useReducer관련해서도 더 알아보게 되었습니다 🙃

commit : 83f23a7

Comment on lines 42 to 43

Choose a reason for hiding this comment

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

변경되지 않을 데이터도 state에 넣어주는게 맞을까요? 👀

Copy link
Author

Choose a reason for hiding this comment

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

맞아요 ㅠㅠ 변경되지 않을 데이터인데 state에 넣어주게 되어서 상당히 찝찝했습니다.. 🥲 저와 페어는 이 문제를 알고 있었지만,, 시간 관계상 적절한 해결책을 내지 못했어요 흑흑

가장 간단한 방법으로 고쳐보았는데, 완벽하지는 않은 것 같네요 🤔
좀 더 고민해보겠습니다!

commit : 990f01a

Comment on lines +51 to +62
const isFullNumber = (number) =>
Object.values(number).every((value) => value.length === INPUT_MAX_LENGTH.NUMBER);

const isFullCompany = (company) => company !== '';

const isFullExpiryDate = (expiryDate) =>
Object.values(expiryDate).every((value) => value.length === INPUT_MAX_LENGTH.EXPIRY_DATE);

const isFullPrivacyCode = (privacyCode) => privacyCode.length === INPUT_MAX_LENGTH.PRIVACY_CODE;

const isFullPassword = (password) =>
Object.values(password).every((value) => value.length === INPUT_MAX_LENGTH.PASSWORD);

Choose a reason for hiding this comment

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

검증로직 깔끔하네요!
every 활용도! 👍 👍

Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

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

몇가지 코멘트를 추가로 남겼는데 또 추가적으로 코멘트 달만한 부분이 보인다면 single comment로 남겨드릴게요! 무비님 화이팅 ✊

Comment on lines +10 to +12
.button-text {
margin-right: 10px;
}

Choose a reason for hiding this comment

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

요런 css를 범용적으로 쓰면 확장성이 떨어지는데 module css나 css in js 라이브러리를 사용하는 방법으로 해결할 수도 있을거같아요 👀

추가적인 방향성만 제시드리려고 추가한 코멘트이고 적용을 권고하는건 어납니다!

Comment on lines 4 to 34
export const cardNumberInputInfoList = [
{
id: uuid(),
type: 'text',
className: 'mr-n15 tracking-wide',
name: 'first',
maxLength: INPUT_MAX_LENGTH.NUMBER,
autoFocus: true,
},
{
id: uuid(),
type: 'text',
className: 'mr-n15 tracking-wide',
name: 'second',
maxLength: INPUT_MAX_LENGTH.NUMBER,
},
{
id: uuid(),
type: 'password',
className: 'mr-n15 tracking-wide',
name: 'third',
maxLength: INPUT_MAX_LENGTH.NUMBER,
},
{
id: uuid(),
type: 'password',
className: 'tracking-wide',
name: 'fourth',
maxLength: INPUT_MAX_LENGTH.NUMBER,
},
];

Choose a reason for hiding this comment

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

배열이 변경되지 않을때에도 uuid를 사용해서 고유하게 key를 생성해줘야할지는 생각해볼 필요가 있겠네요!

Copy link
Author

Choose a reason for hiding this comment

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

아사님이 말씀하시는 배열이 변경되지 않을때가 코드가 실행중일 때라면, 딱히 uuid를 사용해주지 않아도 될 것 같습니다.

언젠가 map의 key에 index 사용을 지양해야한다는 말을 들은 적 있습니다. 그래서 생각없이 uuid를 사용한 것 같습니다. 그런데 지양해야한다는 설명에 index를 안전하게 사용할 수 있는 경우가 따로 존재하네요!

  1. the list and items are static–they are not computed and do not change;
  2. the items in the list have no ids;
  3. the list is never reordered or filtered.

When all of them are met, you may safely use the index as a key.

사실상 index를 key로 사용해주어도 무관하겠네요! index를 key로 사용하도록 변경했습니다.

commit : 081fb70

Comment on lines 3 to 10

Choose a reason for hiding this comment

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

절대경로를 설정해두면 Path를 깔끔하게 관리할 수 있어요~

Copy link
Author

Choose a reason for hiding this comment

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

전혀 몰랐던 부분인데 정말 꿀팁이네요 🤩!!!!!
절대경로로 설정해주니깐 import 구문이 훨배 깔끔해보이네요 ㅎㅎ
그리고 항상 상대경로 계산하기..힘들었는데.. 이런 꿀팁이..!! 애용할 것 같습니다.
빠르게 수정해주었습니다 ~!!

commit : e9f25dd

Comment on lines 1 to 8

Choose a reason for hiding this comment

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

해당 컴포넌트를 열어보지 않으면 children이 어디에 들어갈지 파악하기 어려울것같다는 생각이 드네요 ㅎㅎ

공통 컴포넌트로 사용하러면 헤더에는 왼쪽뿐만 아니라 오른쪽에도 추가적인 UI가 들어가는 경우가 많으니 조금 더 유연하게 작성해보면 좋을거같아요. (물론 이번 미션 요구사항에는 안맞는 설계일수도 있지만 실제로 요구사항이 추가되고 나서야 리팩터링을 한다면 지옥이.. 😈 )

스크린샷 2022-05-01 오후 8 56 13

Copy link
Author

Choose a reason for hiding this comment

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

헉 맞아요! 페어와도 이에 대해 얘기를 했었는데요!
이 부분도 시간 관계상 해결하지 못했네요 🥲

고민해보았는데, 굳이 children을 활용하지 않아도 될 것 같아요.
props로 element를 넘겨주고 props를 leftright로 나누어 오른쪽에 추가적인 UI가 들어가는 것을 대비하는 방법도 괜찮을 것 같아요!
혹시 더 괜찮은 방안이 있을까요? 😁

commit : 5901e05

Choose a reason for hiding this comment

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

요거도 제 의도와 똑같이 반영이 됐네요!
이정도로 충분하다고 생각해요~ 👍

@byhhh2
Copy link
Author

byhhh2 commented May 3, 2022

안녕하세요 아사님!

아직 모든 것을 고치지 못한 상황이지만..🥲 수요일이 STEP1 머지 권장일이기 때문에 리뷰요청 보냅니다! 저는 진짜 거북이인가봐요.. 뭔 속도가..💧🐢

제가 더 적용하고 싶은 부분은 이렇습니다.

  1. 사용자가 옳지 않은 입력을 하려고 할 때 적절한 알림주기
  2. module css 적용하기
  3. input 에 관련된 로직 custom hook으로 분리하기

추가로 수정한 부분은 이렇습니다.

  • memo와 useCallback을 이용하여 렌더링을 최적화해보았습니다. 5b1fe00
    • 잘 몰랐던 부분이라 공부하느라 시간이 좀 오래걸렸네요! 대신 완벽하게(?) 이해하려고 노력했어요.......
  • storybook에 디테일을 추가해주었습니다. 66a7aa1
  • prop type을 지정하지 않은 컴포넌트들을 추가로 지정해주었습니다. d698b90

이번에도 꼼꼼하게 리뷰해주셔서 너무 감사해요 🥺 페어랑 같이 리뷰읽어보면서 맞지맞지 그랬지 ~! 하면서 함께 감탄했답니다 히히

Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

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

무비님 다음 미션에서 도전해볼 부분까지 정리해두셨네요 👏

추가로 수정하신 부분까지 확인했어요!

진짜 거북이인가봐요.. 뭔 속도가..

리액트 미션에 들어와서 새로운 개념이 많다보니 공부할 부분도 많다보니 늦어질 수 밖에 없을거같아요 😅
다음 미션에서 해결해 볼 문제들도 명확하고 바로 다음스텝으로 넘어가도 충분해보이네요~

추가 코멘트도 남겨드렸어요! 확인부탁드리고 이번 미션 고생많으셨어요!

theme: PropTypes.string.isRequired,
};

export default memo(CardCompany);

Choose a reason for hiding this comment

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

memo는 어떤 방식으로 메모리제이션을 할까요? (두번째 인자를 넣지않았을 때)
그리고 memo 2번째 인자를 넣어줄 수 있는데 해당 두번째 인자는 무엇일까요?

메모리제이션을 언제 사용해야할지 한번 더 생각해보는것도 좋을거같아요 🙂

Copy link
Author

Choose a reason for hiding this comment

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

memo는 어떤 방식으로 메모리제이션을 할까요? (두번째 인자를 넣지않았을 때)

memo 는 두번째 인자를 넣어주지 않았을 때, 얕은 비교를 통해 이전 props와 다음 props를 비교합니다. (코드)

비교해서 변경사항이 없다면 감싸진 component를 재렌더링하지 않습니다.

그런데 얕은 비교를 하기 때문에 주의해야할 점이 있습니다.

  • 원시 타입은 만 비교하기 때문에 주의하지 않아도 된다.
  • 하지만 object 타입일 때는 참조를 비교하기 때문에 주의해야한다. (함수 또한 객체다) ⇒ 이러한 이유로 useCallback 을 함께 사용해준다. (이전에 만들어둔 함수 참조를 그대로 사용하기 위해)

그리고 memo 2번째 인자를 넣어줄 수 있는데 해당 두번째 인자는 무엇일까요?

React.memo(Component, [areEqual(prevProps, nextProps)]);

두번째 인자에 직접 만든 함수를 넣어서 memo의 비교방식을 변경할 수 있습니다.! 직접 만든 함수를 활용하면 좋은 점이 비교하고 싶은 특정 값만 비교할 수가 있다라는 점 같습니다. 언제나 true를 반환할 원시 타입 props를 빼주면 될 것 같아요!


메모리제이션을 언제 사용해야할지

부모 state에 자주 변하는 state가 존재하는데 정작 자식은 그 state를 props로 받지 않으며, 자식이 받는 props는 자주 변경되지 않을 때 사용해주면 좋을 것 같아요!

Comment on lines +4 to +8
getCLS(onPerfEntry);
getFID(onPerfEntry);
getFCP(onPerfEntry);
getLCP(onPerfEntry);
getTTFB(onPerfEntry);

Choose a reason for hiding this comment

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

웹 바이탈 용어도 나중에 여유가 되면 공부해보면 어떨까 싶네요 ㅎㅎ
get 메서드 뒤에있는 LCP, CLS, FID 등등이요!

Copy link
Author

Choose a reason for hiding this comment

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

Web Vital : 웹 사용자 환경 지표화 , 퍼포먼스 객관화

CLS : cumulative layout shift (누적 레이아웃 이동)

  • 사용자에게 발생하는 레이아웃 이동 빈도 (사용자에게 컨텐츠가 얼마나 불안정한지 측정)

FID : first input delay (최초 입력 지연)

  • 사용자가 처음 상호작용한 시간부터 처리를 시작할 수 있는 시간까지의 지연

FCP : first contentful paint (최초 콘텐츠풀 페인트)

  • 초기 DOM 컨텐츠를 렌더링하는데 걸리는 시간

LCP : largest contentful paint (최대 콘텐츠풀 페인트)

  • 가장 큰 컨텐츠를 렌더링하는데 걸리는 시간

TTFB : time to first byte (최초 바이트 시간)

  • 페이지를 요청했을 때 서버의 데이터 첫 바이트가 도착하는 시점 (서버 성능)

CLS, FID, LCP는 코어한 Web Vital!

이라고 하네요.. 세상에 세상에 이런 것도 있네요

Comment on lines +15 to +20
const handleChange = useCallback(
(e) => {
onChange(e, item);
},
[onChange, item],
);

Choose a reason for hiding this comment

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

무비님이 완벽하게 이해했는지 검증해보고 싶어졌어요 😈

메모가 어떻게 동작하는지 확인해보고 여기서 handleChange를 useCallback으로 사용하는게 맞을지 생각해보면 좋을거같아요~
memo가 유의미하려면 handleChange가 여기서 useCallback을 하는게 맞을지 다시 생각해봅시다! 🙂

Copy link
Author

Choose a reason for hiding this comment

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

  • 자식 컴포넌트와 관련 없는 state가 변경될 때 자식 컴포넌트 렌더를 막는다 → memo
  • 부모 컴포넌트가 렌더될 때 내부 함수의 주소값이 변경되는 것을 막아서 이를 props로 받는 자식 컴포넌트가 재렌더 되는 것을 막는다 → useCallback

이 정도면 충분하지 ~!! 하면서,,
저 두가지만 보면서..보면서.. 해봤는데 털썩.. 이거 좀 어렵네요.

일단 handleChange<input />태그에만 넘겨주고 있습니다.
그런데 사실 <input />은 memo로 감싸져 있지 않네요?? 이러면 의미가..없는 것 같아요. 어차피 부모가 렌더링되면 <input />도 렌더링 되니깐요. 그런데 이게 이유가 과연 맞을까요,, 너무 헷갈려요 😵‍💫

결과적으로는,, 의미없는 useCallback인 것 같아요 🥲

Copy link

@EungyuCho EungyuCho May 4, 2022

Choose a reason for hiding this comment

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

오 무비 잘 짚으셨는데요~
현재상태로서는 의미없는 useCallback인게 맞아요!

React의 memo가 이전 props을 감지해서 현재 props과 달라졌는지 확인을 하는 과정이 있는데 이 과정에서 이전과 같은 함수를 넣어주기 위해 useCallback을 주로 사용하죠 ㅎㅎ

그래서 React Component가 아닌 일반적인 태그(host element)에 넣어주는 함수에 useCallback은 크게 의미가 없다고 생각해요~
useCallback자체로도 비용이 있기때문에 무작정 사용하는건 바람직지 않다고 봐요.
특히 컴포넌트에서 하위 컴포넌트가 없을 때(리프 컴포넌트) 일때는 useCallback을 더더욱 사용할 필요가 없겠죠? 🤔

결과적으로 memo를 유의미하게 하기위해 사용한다고 보시면 됩니다 🙂
다음번에는 불필요하게 컴포넌트의 리렌더링을 줄이기 위해 제대로 된 메모리제이션을 했는지 검증해보면 될거같아요~
사실 이번 미션에서 메모리제이션을 하지 않아도 성능은 비슷할거라 크게 의미는 없지만.. 이번 미션에서 리렌더링을 어떻게 하면 줄일 수 있는지 생각해보는 과정 속에서 다음에 성능 최적화를 하시거나 메모리제이션을 했는데 왜 또 렌더링이되는지 이해가 안될때 조금 더 빨리 디버깅을 할 수 있는 기초체력이 되어줄거라 생각해요 😀

참고1: useCallback과 memo를 통한 렌더링 최적화
참고2: useCallback, useMemo 정확하게 사용하고 있을까
참고3: 참조 동일성을 위한 메모리제이션

추가적으로 리액트 렌더링에 대해 deep하게! 자세히! 알아보고 싶으시다면 reconciliation, fiber까지 키워드를 익혀두시면 언젠가 면접에서 쓰시지 않을까 싶네요 ㅎㅎㅎ..

fiber 관련 글

메모리제이션이 이해가 쉽지 않았을텐데 고생하셨어요!
역시 갓무비 👍 👍

Copy link
Author

Choose a reason for hiding this comment

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

앗! 제가

특히 컴포넌트에서 하위 컴포넌트가 없을 때(리프 컴포넌트) 일때는 useCallback을 더더욱 사용할 필요가 없겠죠? 🤔

이 부분을 간과하고 있었네요 ㅎㅎㅎㅎ!!!
역시 ✨갓아사✨..참고 링크까지.. 주륵 🥲
코피 뽑더라도 공부 갑니다~!!~~!~!

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

Comments