[윤정환] sprint6

[khuyjh]

배포

https://sprint-mission6.netlify.app/

요구사항

기본

• 상품 등록 페이지 주소는 “/additem” 입니다.
• 페이지 주소가 “/additem” 일때 상단네비게이션바의 '중고마켓' 버튼의 색상은 “3692FF”입니다.
• 상품 이미지는 최대 한개 업로드가 가능합니다.
• 각 input의 placeholder 값을 정확히 입력해주세요.
• 이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.

심화

• 이미지 안의 X 버튼을 누르면 이미지가 삭제됩니다.
• 추가된 태그 안의 X 버튼을 누르면 해당 태그는 삭제됩니다.

주요 변경사항

-미션5 리뷰를 바탕으로 리팩토링도 진행했습니다.

스크린샷

멘토에게

• 미션 6 부분의 폴더구조를 기존과 다르게 했는데, 이전 미션에 작성한 부분들은 폴더를 옮기면 리뷰에 방해될 것 같아서 아직 구조를 바꾸지 않았습니다. 리뷰 받고 머지요청 전에 변경하겠습니다.
• 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

[addiescode-sj]

수고하셨습니다!

주요 리뷰 포인트

• 불필요한 리렌더링 방지 (form state update, useEffect 내부 함수 호출 메모이제이션)
• 커스텀 훅 재사용성 향상
• z-index 범주화
• 리턴문 간소화

[addiescode-sj]

조건문이 너무 복잡해보이네요! 리턴문은 최대한 명확하고 간결하게 유지해봅시다.

  const isMarketPage =
    location.pathname === "/items" || location.pathname === "/additem";

Suggested change

	className={`btn ${styles.link} ${
	styles[
	`${
	location.pathname === "/items" ||
	location.pathname === "/additem"
	? "selected"
	: "none"
	}`
	]
	className={`btn ${styles.link} ${
	isMarketPage ? styles.selected : styles.none
	}`}

이정도로 정리해보는건 어떨까요? :)

[addiescode-sj]

z-index는 예상치 못한 충돌을 방지하고 일관성을 높이기위해 범주화해서 사용하는것이 좋습니다.

이런식으로 css 변수를 만들어서 하드코딩한 숫자말고 미리 정해둔 범위 안에서만 숫자를 사용해보도록할까요?

:root {
  /* 기본 레이어 */
  --z-index-base: 0;
  
  /* 콘텐츠 레이어 */
  --z-index-content: 1;
  
  /* 상호작용 요소 */
  --z-index-interactive: 10;
  
  /* 드롭다운, 툴팁 */
  --z-index-dropdown: 100;
  
  /* 모달 오버레이 */
  --z-index-overlay: 200;
  
  /* 모달 */
  --z-index-modal: 300;
  
  /* 토스트, 알림 */
  --z-index-toast: 400;
  
  /* 네비게이션 */
  --z-index-navigation: 500;
  
  /* 최상위 레이어 */
  --z-index-top: 999;
}

이렇게 범주화해보면 레이어구조를 한눈에 파악할수있다는 장점도 생기겠죠?

[addiescode-sj]

리액트 컴포넌트를 제외하고, 커스텀훅은 파스칼케이스로 작성하실 필요 없습니다 :) 파일명을 usePagination으로 맞춰주세요!

[addiescode-sj]

BUTTONS_PER_PAGE를 default value로 만들고 props로 추가한다면 좀더 로직의 재사용이 유연해지겠죠? 로직의 재사용을 위해서는 UI 관련 규칙으로부터 최대한 종속성을 제거하는편이 좋습니다! :)

[addiescode-sj]

default 케이스에서는 필요한 반환값이 빠져있는것같은데 좀 더 안정적인 동작을 위해 기본 반환값을 묶는 객체를 만들고 spread 연산자를 활용해보는건 어떨까요?

예시)

  // 기본 반환값
  const baseReturn = {
    value,
    displayedValue,
    isFocused,
    handleFocus,
    handleBlurDisplayedValue,
  };

  switch (id) {
    case "productPrice": {
      return {
        ...baseReturn,
        handleChangePrice,
      };
    }
    default: {
      return {
        ...baseReturn,
        handleChangeValue,
      };
    }
  }

[addiescode-sj]

useEffect 내부에서 호출되는 함수가 있다면 해당 함수를 메모이제이션하고 deps list에 넣어주시는게 좋습니다! 만약 onSaveData 함수가 매번 부모에서 새로 인스턴스가 생긴다면 무한 리렌더링이 발생해 성능에 좋지 않기때문입니다.

부모에서 먼저 useCallback을 사용해 메모이제이션을 적용해주고

  const handleSaveData = useCallback((updater) => {
    setFormData(updater);
  }, []);

deps list에 onSaveData를 추가해봅시다 :)
이렇게 하면 함수 참조가 좀 더 안정적으로 동작해 불필요한 useEffect 실행 및 무한 렌더링 현상을 방지할수있고, 언제 해당 useEffect가 실행될지 예측하는게 수월해집니다.

해당 내용은 공식 홈페이지에서도 권장하고있는 내용입니다 :)

https://react.dev/reference/react/useCallback#preventing-an-effect-from-firing-too-often

[khuyjh]

onSaveData는 formData의 setter인 setFormData를 내려주는 프롭인데, setter는 useCallback 적용이 필요없다고 들어서 적용해주지 않았었습니다. 혹시 제가 잘못 알고있을까요..?

[addiescode-sj]

그랬군요! 제가 파일 변경 내역에서 보이는 코드만 보고 리뷰 드렸네요 ㅎㅎ useState의 setter 함수는 컴포넌트가 리렌더링되어도 참조가 변경되지않기때문에 의존성 배열에 추가하지않고 메모이제이션하지않아도 괜찮습니다. useEffect 내부에서 setter가 아닌 일반 함수를 호출할 경우에 이런 변경이 필요하다는점만 알고가시면 좋을것같아요 :)

[addiescode-sj]

현재 코드에서 formData 객체를 state로 관리하고 있어서, 하나의 필드값만 변경되어도 전체 객체가 새로 생성되기때문에 불필요한 리렌더링이 발생하는 문제가 생깁니다.

deps list에 formData state가 있는데, formData가 변경될 때마다 useEffect를 실행하고, 하나의 필드만 변경되어도 전체 formData가 새로 생성되기때문입니다.

이 현상을 방지하려면 필드들을 개별 state + handler를 사용하도록하거나
useReducer를 사용해 선택적으로 구독하게끔하거나
ref기반으로 바꾸는 방법들이 있습니다.

리팩토링시에는 개별 state + handler를 사용하는걸로 바꿔보시고
아래 아티클보고 ref기반으로 폼 컨트롤 하는 방식(react-hook-form 라이브러리 접근법 )에 대해 공부해보시면 좋을것같네요 :)

ref와 비제어 컴포넌트, react-hook-form 소개
vanillaJS|TS로 react-hook-form처럼 react form관리 만들기

[addiescode-sj]

이렇게 해주시는게 좀 더 간소하겠네요 :)

Suggested change

	{hasError ? (
	<div className={styles.errorMessage}>{ERROR_MESSAGE}</div>
	) : undefined}
	{hasError && (
	<div className={styles.errorMessage}>{ERROR_MESSAGE}</div>
	)}