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

경북대 FE_이효은_2주차 과제 Step1~3 #14

Open
wants to merge 78 commits into
base: hyoeunkh
Choose a base branch
from

Conversation

Hyoeunkh
Copy link

@Hyoeunkh Hyoeunkh commented Jul 4, 2024

안녕하세요 멘토님!
지난 1주차 레포를 히스토리와 함께 가져와서 commit 기록이 모두 남아있습니다.
2주차 코드는 e0a52fc 부터이고,
포매팅 제외한 실제 구현 코드는 4080eb1 부터입니다.

1주차 레포에도 리뷰 코멘트 남겨두었으니 확인 부탁드립니다! 감사합니당


<궁금한점>

  1. 필터 기능을 구현하는데 Prop drilling이 너무 심해서 이 코드가 맞나 싶습니다. 깔끔하게 구현하는 방법이 있을까요?,,

  2. 2단계 과제에 모든 페이지 진입 시 authToken을 토대로 로그인 여부를 판단하는 로직을 추가해요. (ContextAPI 활용)라고 되어있는데, 결과물 링크를 보면 ThemePage는 로그인하지 않아도 진입이 됩니다. 그래서 특정 경로에서는 로그인 없이 진입이 안되도록 RequiredLogin이라는 로직을 추가했는데, 이 의도가 맞는지 궁금합니다. 그리고 이런 추가적인 라우팅(?) 로직은 어떤 폴더에 넣어야 할까요? 현재는 utils에 넣어두었습니다!

  3. 현재 components/feature/Home 에 HomePage에서 사용되는 모든 컴포넌트가 들어가있는데, 코드 수정하면서도 기능마다 구분하기 번거로워서 어떤 식으로 폴더구조를 잡아야 할지 모르겠습니다!

Copy link

@sjoleee sjoleee left a comment

Choose a reason for hiding this comment

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

안녕하세요!

정말 잘 구현해주셔서 코드상의 세세한 문법 수정같은 리뷰를 남기는 것은 큰 의미가 없다고 생각해요.
작성해주신 고민에 대해서 이야기하는게 훨씬 도움이 될 것 같아요

필터 기능을 구현하는데 Prop drilling이 너무 심해서 이 코드가 맞나 싶습니다. 깔끔하게 구현하는 방법이 있을까요?,,

prop drilling이 심하다면 굳이 prop으로 내려주지 않아도 상태에 접근할 수 있도록 context나 전역 상태 관리 도구를 사용하면 되겠죠?

근데 GiftRanking > RankingFilter > FilterList 정도인데 '너무 심하다'고 볼 수 있을까요...?
저는 필요하다면 내려주자는 주의라서 그렇게 심하다는 느낌은 없네요.
복잡도가 높은 원인이 prop drilling이 아닐 수도 있지 않을까요? 컴포넌트를 리팩토링 하면서 복잡도를 개선해 보면 어떨까 싶어요.
아래에 코드 조각들을 몇개 작성해둘게요. 보시면서 어떻게 활용하면 복잡도가 낮게 설계할 수 있을지 고민해 보시면 좋겠습니다.

<Container>
  <Filter />
  <FilteredList />
</Container>

Target과 Type에 사용될 데이터는 이런 형태가 되지 않을까 싶어요.

export const TARGET_FILTER_ITEMS = [
  { icon: 'ALL', text: '전체' },
  { icon: '👩🏻', text: '여성이' },
  { icon: '👨🏻', text: '남성이' },
  { icon: '👦🏻', text: '청소년이' },
] as const;

export const TYPE_FILTER_ITEMS = [
  { text: '받고 싶어한' },
  { text: '많이 선물한' },
  { text: '위시로 받은' },
] as const;

그리고 Target과 Type을 별도의 state가 아니라 하나의 객체타입 state로 관리하면 조금 더 간결하게 표현할 수 있겠죠?

type Target = (typeof TARGET_FILTER_ITEMS)[number]['text'];
type Type = (typeof TYPE_FILTER_ITEMS)[number]['text'];

interface Filter {
  target: Target;
  type: Type;
}

const [filter, setFilter] = useState<Filter>({ target: '전체', type: '받고 싶어한' });

  const changeFilter = ({ target, type }: Partial<Filter>) => {
    setFilter((prev) => ({ target: target ?? prev.target, type: type ?? prev.type }));
  };

이렇게 사용하면 선언된 상수의 text들이 유니온 타입으로 만들어져 filter 상태의 target과 type을 안전하게 사용할 수 있어요.

<Container>
  <Filter filter={filter} changeFilter={changeFilter} />
  <FilteredList filter={filter} />
</Container>
// <Filter filter={filter} changeFilter={changeFilter} />

const onTargetChange = (target: Target) => {
  changeFilter({ target });
}

const onTypeChange = (type: Type) => {
  changeFilter({ type });
}

  <TargetFilter targetValue={filter.target} onTargetChange={onTargetChange} />
  <TypeFilter typeValue={filter.type} onTypeChange={onTypeChange} />
// <FilteredList filter={filter} />

const filteredItems = goodsItems.filter((goods) => {
  return (filter.target === '전체' || goods.filterTarget === filter.target) && goods.filterType === filter.type;
});

const visibleItems = more ? filteredItems : filteredItems.slice(0, 6);

  <List items={visibleItems} /> 
  <button ... /> 

이런 느낌으로 설계하면 단순히 자손으로 전달하기 위해 prop을 그대로 넘기는 게 아니라, 각 컴포넌트의 역할이 명확해지고 필요한 prop을 전달받고 있으며, 프로그래매틱하게 뷰를 그릴 수 있을 것 같아요.

2단계 과제에 모든 페이지 진입 시 authToken을 토대로 로그인 여부를 판단하는 로직을 추가해요. (ContextAPI 활용)라고 되어있는데, 결과물 링크를 보면 ThemePage는 로그인하지 않아도 진입이 됩니다. 그래서 특정 경로에서는 로그인 없이 진입이 안되도록 RequiredLogin이라는 로직을 추가했는데, 이 의도가 맞는지 궁금합니다. 그리고 이런 추가적인 라우팅(?) 로직은 어떤 폴더에 넣어야 할까요? 현재는 utils에 넣어두었습니다!

의도는 잘 파악하신 것 같아요. 다만, 페이지 라우팅을 처리하는 로직을 한 군데에 모아두면 좋을 것 같습니다.
굳이 App.tsx에서만 사용될 로직을 다른 폴더로 옮겨야 할까요?

const ProtectedRoute = ({ children }: PropsWithChildren) => {
  const { authToken } = useAuth();
  return <>{authToken ? children : <LoginPage />}</>;
};

<Route
  path="/my-account"
  element={
    <ProtectedRoute>
      <MyAccountPage />
    </ProtectedRoute>
  }
/>

현재 components/feature/Home 에 HomePage에서 사용되는 모든 컴포넌트가 들어가있는데, 코드 수정하면서도 기능마다 구분하기 번거로워서 어떤 식으로 폴더구조를 잡아야 할지 모르겠습니다!

폴더 구조가 약간 어색한 것 같아요. 예를 들면, features 하위에 페이지가 오면 기능 단위로 구분된 것이 아니게 됩니다.

차라리 components 하위에 페이지별 구분을 두거나,

components
 ┣ common
 ┃ ┣ Form
 ┃ ┣ Input
 ┣ Home
 ┃ ┣ GiftRanking
 ┃ ┃ ┗ RankingFilter
 ┃ ┗ AIGift
 ┣ Login

features 폴더를 components와 동일한 위계에 두는 것은 어떨까요?

components
 ┣ Form
 ┣ Input
features
 ┣ Home
 ┃ ┣ GiftRanking
 ┃ ┃ ┗ RankingFilter
 ┃ ┗ AIGift
 ┣ Login

최근에는 fsd 아키텍처가 유행하는 것 같은데.. 저는 개인적으로 프로젝트 규모가 크지 않으면 페이지 단위로 폴더링하는 것이 가장 무난하다고 생각해요.

Comment on lines 13 to 15
if (!authToken) {
return <Navigate to="/login" replace={true} />;
}
Copy link

Choose a reason for hiding this comment

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

페이지 내에서 개별적으로 관리하기보다는 추상화하여 선언적으로 접근제한을 관리하는 것이 좋지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

62afe2f

const ProtectedRoute = ({ children }: PropsWithChildren) => {
  const { authToken } = useAuth();
  return <>{authToken ? children : <LoginPage />}</>;
};

이런식으로 <LoginPage />를 return 해주니 url이 바뀌지 않아서
/login으로 이동하도록 수정해서 커밋했습니다 !
감사합니다

<>
<SelectFriend />
<ThemeCategory />
<AIGift />
Copy link

Choose a reason for hiding this comment

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

개발의 8할은 네이밍이라는 말이 있듯이 굉장히 어렵고도 중요한 일이라고 생각해요.
AIGift이 해당 영역을 정말 정확히 드러내는 이름일까요?
예를 들면, AIRecommendedGiftBanner 처럼 더욱 명확한 네이밍이 협업할때 좋지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

0cd3da2 컴포넌트 이름 수정했습니당

Comment on lines 18 to 21
const location = useLocation();
const hideLayout = location.pathname === '/login';

if (hideLayout) return <LoginPage />;
Copy link

Choose a reason for hiding this comment

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

이런 로직이 반드시 필요할까요?
해당 로직의 목적이 '로그인 페이지는 레이아웃을 적용하지 않기 위함'이라면 레이아웃 컴포넌트 내에서 조건부로 렌더링하기보다 아예 로그인 페이지를 레이아웃 컴포넌트 바깥으로 빼는 방법이 의도를 명확히 드러낼 수 있을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

9ee5a84 감사합니다!

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.

3 participants