[클린코드 리액트 2기 양세영] 페이먼츠 미션 Step 3 #97
Conversation
Step3 merge
seyoungjoy
changed the title
There was a problem hiding this comment.
안녕하세요 세영님
스텝3까지 무사히 진행해주셨군요..!
카드 등록 페이지에 있는 input을 모두 제어 컴포넌트로 관리하다보니 input값 하나하나에 액션을 취할 순 있지만 그만큼 제가 챙기지 못하고 있는 성능적인 관점이 걱정이 됩니다.. 잘 모르고 쓰면 오히려 독이라 해서 usememo, usecallback을 전혀 안쓰고 있었는데 공부해서 한번 개선시켜보고싶네요!
넵 제 생각에도 useMemo, useCallback은 잘못 사용하면 독이라고 생각하긴해요.
제가 여러 성능 개선을 한다고 난리를 몇 번 쳐봤었는데 그럴 때 마다 느낀 점은 "웬만한 프로젝트에서 성능 개선은 그림의 떡 같은 느낌인 것 인가?.. 배보다 배꼽이 더 큰 것인가?.." 하는 생각을 매번 했어요.
성능 개선이 사실은 사람들이 면접 질문에서 "성능 개선을 해보셨나요?" 뭐 이런 질문들이 무서워서 성능 개선을 하지도 않아도 되는 곳에 성능 개선을 한다고 하다보니 상황이 이렇게 된 것 같은 느낌이기도 해요. (조금 쎄게 말했지만 사실이라고 생각합니다.) 그래서 제 생각에 성능 개선은 성능 개선이 필요하다고 느끼는 시점에 하면 된다고 생각해요. 예를들어 한 페이지에 인풋이 100개가 있는데 하나를 입력할 때마다 나머지 100개의 인풋이 리렌더링 되면 그건 속도가 심각하게 저해될거에요. 좀 극단적인 예시지만 성능 저하가 확실한 곳에서는 성능 개선이 이뤄주는 것들이 많을거에요. 하지만 단순 페이지에서의 성능 개선은 큰 의미는 없다고 생각해요. useCallback이나 useMemo 같은 함수들도 말 그대로 함수라서 실행이 되고 어떤 로직이 수행이 되는거라서 마법같은게 아니거든요.
하지만 공부를 하는 입장에서 적용을 해보고 이게 어떻게 사용되는지 확인해보는 모습은 저는 당연하게 좋다고 생각합니다. 콘솔로그를 찍어가시면서 두 개의 훅을 사용해보고 어떤 결과가 있는지 확인해보는 건 저도 좋다고 생각해요ㅎㅎ
CardContext.tsx 파일에 효율적으로 코드가 짜여져 있는지
저번 피드백주신부분 참고해서 잠깐 헤어졌던 reducer함수를 다시 불러왔구요!(작업하다보니 왔다갔다하는게 정말 더 불편하더라구요!) 그리고 validation context를 해당 파일에 추가하면서 코드가 좀 복잡해진것같아서 개선할 수 있는 점이 있을지 궁금합니다!
오호 CardValidationContext 를 하나 더 만드셨군요.
저는 생각을 해봤는데 되게 좋은 접근 같아요.
컨텍스트를 나눈다는 것을 조금 깊게 고민해봤는데 "관심사를 분리한다" 혹은 "관리 포인트의 범위를 분리한다" 정도로 볼 수 있는 것 같은데요. 그런 관점에서는 적당한 분리 같습니다.
한 가지 주의해야 할 것 같은건 CardValidationContext 이랑 CardStateContext은 항상 같이 움직인다는 거에요. 이게 무슨 말이냐면 CardValidationContext는 CardStateContext에 항상 의존하고 있을 것 같아요. state가 변할 때 validation이 일어나는 법이니까요.
저는 항상 같이 붙어있으면 굳이 나누지 않는다는 생각을 가지고 있지만,
이 생각은 오직 저의 의견이고 정답이 아니라는 거에요. 다르게 생각하는 분도 있을 수 있고
충분히 환경과 시대와 언어가 발전함에 따라서 또 달라질 수 있을 것 같네요.
하지만 세영님이 분리해주신 validation은 충분히 좋은 시도였고, 괜찮았다고 저는 생각해요.
DX 라는 것 들어보셨나요, Development Experience로 개발 경험을 말하는건데 저는 이것도 되게 중요한 부분이라고 생각하거든요. 제가 위에서 말씀드린 부분도 DX의 일종이기도 하고 어떻게 보면 성능적인 측면도 있을 수 있겠죠 (항상 같이 있고 함께 변경되는데 분리시켜놓으면 코드가 무엇인가를 더 한다는 얘기잖아요?)
하지만 세영님의 분리는 DX적인 측면에서 좋은 시도였다고 생각합니다. 세영님이 주도적으로 생각을 가지고 코드를 짜 나가다 보면 점점 코드에 정답은 없고, 주도적으로 코드를 짜실 수 있을거에요. 저도 그 정도 경지까지는 가지 못했지만 화이팅해보자구요.
카드사 추정 및 유효한 값 자동 탭 기능
해당 기능들이 사용자 입력값에 실시간으로 대응해야하다보니 cardForm.tsx 에 모두 들어가게 되었는데 너무 복잡해보이는지...? 궁금합니다!
흠 괜찮아 보이긴 한데 로직을 더 나눌 수 있겠다는 생각을 했어요.
cardForm은 정말로 UI 컴포넌트로 데이터를 받아서 뿌려주는 역할만 하고
useEffect나 showCompanySelectModal 같은 함수의 로직은 조금 더 훅에 위임할 수 있다고 생각은 하지만 그래도 이 정도도 저는 충분히 좋다고 생각합니다.
세영님의 고민이 충분히 잘 담겨있는 것 같아서 제가 리뷰하면서도 재미있고 좋더라구요.
모달 컨텍스트
전체에서 사용하기 위해 ModalContext를 추가하고 Modal.tsx에서 컨텐츠들을 관리하는 방향으로 구조를 잡아봤는데 괜찮은지 궁금합니다!
넵 좋은 접근인 것 같아요!
한 가지 챙겨주셨으면 하는 점은 리액트 Portal 개념을 사용하셔서 모달을 리액트 컨텐츠 내부에 속하게 하는 것이 아닌 리액트 앱 밖에 UI가 존재하게끔해서 모달의 스타일이 리액트 앱 안에 갇혀있지 않게 하는 기법인데요 조금 어렵게 설명드렸죠?..
조금 쉽게 말씀드리면
<div>
<div id="react-app">
<-- 세영님이 작성하신 리액트 앱 -->
</div>
<div id="portal">
<-- 모달이 위치하게 될 영역 -->
</div>이렇게 해주는 방식이에요. 그래서 리액트 안의 내부 영역에서 작성하는 코드처럼 보이지만,
외부의 영역으로 포탈을 타고 보내버린다. 라고 해서 portal로 이름이 지어졌을 거에요 (제 뇌피셜입니다.)
그래서 리액트 공식문서에 portal의 개념을 공부하시고 모달을 포탈로 한 번 보내주시죠!
이렇게 하면 아마 zIndex 신경을 크게 쓰지 않아도 원하는대로 스타일링이 잘 되실거에요.
이렇게 포탈을 자주 사용하는 컴포넌트의 예시로는 백드롭(backdrop)이 있는 친구들
지금과 같은 modal, bottom sheet, alert dialog, toast 등이 있겠네요!
포탈의 위치는 제가 코멘트에 간단하게 남겨놨어용!
세영님 고생하셨어요.
우선은 이거 이대로 승인을 해놓을게요!
충분히 괜찮은 것 같아서요.
다음 미션 진행하셔도 되고, 아니면 요기서 질문 더 하실 것 있으시면 해주세요.
이거 한 하루이틀 놔두다가 제가 머지를 하겠습니다ㅎㅎ 감사해요 세영님!
| const useNextFocus = () => { | ||
| const inputRefs: InputRefType = [ | ||
| useRef(null), | ||
| useRef(null), | ||
| useRef(null), | ||
| useRef(null), | ||
| ]; |
There was a problem hiding this comment.
현재 이 useNextFocus 는 4개의 input에서 밖에 대응이 안될 것 같아요!
유동적으로 useNextFocus 에 매개변수를 받을 수 있도록 만들어서 훅을 조금 더 범용성 있도록 만들면 좋을 것 같네요! 훅의 핵심 개념은 "로직을 분리한다." 이긴하지만 그래도 "재사용 가능하면서 로직을 분리한다." 면 더 좋지 않을까요?
지금은 어느 한 유즈케이스에서 밖에 사용이 불가능 할 것 같네요!
예시를 들어서 제가 예전에 슬랙에 동호님 리뷰하면서 제가 직접 포커스 이동하는 훅을 짧게 구현한 채팅이 있을거에요! 슬랙에도 있구요.
import type { RefObject } from "react";
import { useEffect, useState } from "react";
type Value = {
name: string;
ref: RefObject<HTMLInputElement>;
value: string;
};
// 커스텀 훅을 사용하는 측에서 넘겨줘야 하는 인자들
interface UseInputProps {
values: Value[]; // Input의 name, ref, value를 담은 배열
maxLength: number; // Input의 최대 길이, 해당 길이를 넘으면 focus를 다음으로 넘긴다.
}
export const useInput = ({ maxLength, values }: UseInputProps) => {
const [inputValues, setInputValues] = useState(values); // Input의 name, ref, value를 담은 배열
const [order, setOrder] = useState(0); // 현재 focus가 있는 input의 index
// Input의 value가 변경될 때마다 실행되는 함수
const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
const { value } = event.target as HTMLInputElement;
// 현재 focus가 있는 input의 value를 변경한다.
const newValues = inputValues.map((inputValue) => {
if (inputValue.name === event.target.name) {
return {
...inputValue,
value,
};
}
return inputValue;
});
setInputValues(newValues);
// -> maxLength를 넘으면 order를 1 증가시킨다.
if (value.length === maxLength) {
const next = order + 1;
if (next < inputValues.length) {
setOrder(next);
}
}
};
// -> order가 변경될 때마다 focus를 변경한다.
useEffect(() => {
const input = inputValues[order];
if (input) {
input.ref.current?.focus();
}
}, [order]);
return { handleChange, inputValues };
};위가 그 때 구현한 훅인데 로직적으로 더 다듬어야 하는 부분도 많고 신경써야 하는 부분도 많지만
저런 식으로 받아서 사용할 수 있다? 라는 것을 얘기하고 싶었어요!
There was a problem hiding this comment.
오.. 귀한 자료를 놓칠뻔했네요 감사합니다! 저는 단순하게 nextfocus에 초점을 맞췄는데 input에 관한 로직이니깐 useInput 훅을 만들어서 함께 사용하는것도 더 명료하고 재사용성측면에서도 좋네요..!
|
|
||
| const CardRegisterForm = () => { | ||
| const dispatch = useCardDispatch(); | ||
| const { digits, expire, name, cvc, passwords } = useCardState(); | ||
| const validation = useCardValidation(); | ||
| const [isSelectCompany, setSelectCompany] = useState(false); |
There was a problem hiding this comment.
저는 이 회사 선택 상태값이 useCardState의 company 값으로 판단할 수 있는 것 아닌가? 하는 단순한 생각을 했어요! (꼭 필요한 상태가 아닐 것 같다는 생각)
There was a problem hiding this comment.
헉 맞네요 company가 선택된 상태값이 필요해서 새로 만든거였는데 실컷 만들어두고 안쓰고있었네요ㅠ
| const DIGIT_COMPANY_LENGTH = 8; | ||
| const showCompanySelectModal = () => { | ||
| const digitLength = | ||
| String(digits.digit1).length + String(digits.digit2).length; |
There was a problem hiding this comment.
저는 프로젝트 내에서 반복사용되지 않는 상수 이렇게 가까이 두는거 정말 좋은 것 같아요
There was a problem hiding this comment.
현수님이 알려주신 관리의 측면에서 생각했을 때 가까이 두고 수정하는게 더 좋을거같다고 저도 생각했어요!ㅎㅎㅎ
| .value, | ||
| }, | ||
| }); | ||
| if (digits.digit3 || digits.digit4) { |
There was a problem hiding this comment.
뭔가 지금 간단히 생각해봤는데 digits.thridSection digits.fourthSection 이런식으로 조금 더 명시적으로 이름을 지을 수 있겠다고 생각을 했어요
There was a problem hiding this comment.
오 훨씬 좋은거같아요 사실 단순한 숫자가 계속 마음에 걸렸는데 좋은 네이밍 추천 감사합니다!!
| <> | ||
| <S.ModalDimmed | ||
| isShow={isShow} | ||
| onClick={() => setModalState({ type: null, isShow: false })} | ||
| /> | ||
| <S.Modal isShow={isShow}> | ||
| <S.Content>{content[type as ModalType]}</S.Content> | ||
| </S.Modal> | ||
| </> |
There was a problem hiding this comment.
포탈의 위치는 아마 요기의 부모로 감싸주시면 될거에요!
포탈의 자식들이 포탈을 타고 여행을 떠난다. 라고 생각하면 이해하기가 조금 더 쉽겠네용
| <> | |
| <S.ModalDimmed | |
| isShow={isShow} | |
| onClick={() => setModalState({ type: null, isShow: false })} | |
| /> | |
| <S.Modal isShow={isShow}> | |
| <S.Content>{content[type as ModalType]}</S.Content> | |
| </S.Modal> | |
| </> | |
| <Portal> | |
| <S.ModalDimmed | |
| isShow={isShow} | |
| onClick={() => setModalState({ type: null, isShow: false })} | |
| /> | |
| <S.Modal isShow={isShow}> | |
| <S.Content>{content[type as ModalType]}</S.Content> | |
| </S.Modal> | |
| </Portal> |
There was a problem hiding this comment.
자식들이 포탈을 타고 밖으로 슈웅 나가버리는거군요ㅋㅋㅋ 유용한 API 알려주셔서 감사합니다!
| return { | ||
| ...state, | ||
| digits: { | ||
| ...state.digits, | ||
| [action.target.name]: maxLengthCheck(action.target.value, 4), | ||
| }, |
There was a problem hiding this comment.
리듀서의 장점 불변성
불변성이라는게 별거없죠? 그냥 스프레드로 이전의 상태들이 보장이 되면 그게 불변성인 것 같아요ㅋㅋ
잘 해주셨어요!
There was a problem hiding this comment.
덕분에 리듀서 중복코드는 줄이고 깔끔하게 코드짜는법에 대해서 정말 많이 배웠습니다... 감사합니다ㅠㅠ!!
There was a problem hiding this comment.
오우 확실히 처음봤을 때 보다 코드가 좋아진 것 같아요 (제 기준)
저의 피드백을 열심히 반영하시더니 저랑 코드가 비슷해져 가시는군요ㅋㅋㅋㅋㅋㅋㅋㅋ
세영님 고생하셨씁니다!
There was a problem hiding this comment.
ㅎㅎ현수님 리뷰 덕분에 코드를 짤 때 다른 사람이 내 의도를 알 수 있도록 글을 쓴다?! 라는 느낌으로 바뀐것같아요!! 그리고 코드에서 고민의 흔적들이 보였다는 점도 정말 신기했어요... 한글로 쓴 것도 아닌데 그게 보였다니..!!ㅋㅋㅋ
좋은 코멘트 아낌없이 주셔서 정말 많이 배우고 갑니다... 감사합니다!!
| useEffect(() => { | ||
| inputRefs[0].current?.focus(); | ||
| }, []); |
There was a problem hiding this comment.
이 부분도 useNextFocus 에서 해줄 수 있는 로직의 마지노선 이라고 생각하긴 해요!
useNextFocus라는 훅이 현재는 포커스의 로직만 처리해주겠다는 의지인 것 같은데 그러면 이 로직도 충분히 들어갈 수 있는 부분 같아서요!
There was a problem hiding this comment.
맞네요 포커스 관련 훅으로 같이 들어갈 수 있는거네요! 로직이 어느 관심사랑 연결이 됐는지 잘 생각하면서 코드를 짜야겠어요..!
Pull Request Template
1. 가이드
1-1. 배포
1-2. 로컬 환경
2. 리뷰 요청 ✍️
2-1. 어렵거나 아쉬웠던 점
2-2. 나누고 싶은 고민
2-3. 중점적으로 리뷰받고 싶은 부분
CardContext.tsx파일에 효율적으로 코드가 짜여져 있는지저번 피드백주신부분 참고해서 잠깐 헤어졌던 reducer함수를 다시 불러왔구요!(작업하다보니 왔다갔다하는게 정말 더 불편하더라구요!)
그리고 validation context를 해당 파일에 추가하면서 코드가 좀 복잡해진것같아서 개선할 수 있는 점이 있을지 궁금합니다!
해당 기능들이 사용자 입력값에 실시간으로 대응해야하다보니
cardForm.tsx에 모두 들어가게 되었는데 너무 복잡해보이는지...? 궁금합니다!전체에서 사용하기 위해 ModalContext를 추가하고
Modal.tsx에서 컨텐츠들을 관리하는 방향으로 구조를 잡아봤는데 괜찮은지 궁금합니다!4. 요구 사항 ✅
필수 요구사항
UX/UI 요구사항