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

PostMenu, CommentMenu을 하나의 Menu 컴포넌트로 통합 #708

Merged
merged 6 commits into from
Oct 12, 2023

Conversation

inyeong-kang
Copy link
Member

@inyeong-kang inyeong-kang commented Oct 4, 2023

🔥 연관 이슈

close: #687

📝 작업 요약

기존의 PostMenu, CommentMenu을 Menu 컴포넌트 하나로 통합하였습니다.

⏰ 소요 시간

2시간

🔎 작업 상세 설명

  • components/CommentList/CommentMenu 폴더 및 components/post/PostMenu 삭제
  • components 에 있던 type 또는 constant를 src/types, src/constants 폴더로 이동
  • components/PostMenu 생성 (댓글의 메뉴, 게시글의 메뉴 모두에 쓰이도록 범용적으로 구성)

🌟 논의 사항

  • 제가 생각하기에 Menu 컴포넌트는 common 폴더에 위치하는게 좀 더 적절하지만, 기존에 만든 타입(PostAction, PostMenuItem 등)을 재활용하며 타입의 안정성을 확보하고 싶다면 지금처럼 PostMenu의 props를 유지해야 할 것 같습니다.
  • 만약 타입의 안정성보다는 범용성을 챙긴다면, (앞으로 PostAction 외에도 다른 타입과 원활하게 호환되길 원한다면) 좀 더 범용적인 타입으로 리팩터링이 필요하다는 판단이 드네요. 예를 들어
interface MenuItem {
  content: string;
  color: MenuColor;
  action: string;
}

interface PostMenuProps {
  menuList: MenuItem[]; // PostMenuItem 대신 MenuItem 으로
  handleMenuClick: (menu: string) => void; // 기존에는 (menu: PostAction) => void; 
}

export default function PostMenu({ menuList, handleMenuClick }: MenuProps) {
... (중략)
  1. 타입을 범용적으로 바꾸자(그러면 기존 타입이 무용지물이 될 수 있음)
  2. 아니다 그냥 지금처럼 PostMenu props를 유지하여 타입의 안정성을 높이자

여러분의 생각이 궁금합니다! 혹은 더 나은 제안이 있다면 알려주시면 감사하겠습니다~

@inyeong-kang inyeong-kang self-assigned this Oct 4, 2023
@inyeong-kang inyeong-kang changed the title Refactor/#687 PostMenu, CommentMenu을 하나의 Menu 컴포넌트로 통합 Oct 4, 2023
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

⚡️ Lighthouse report!

Category Score
🟠 Performance 71
🟠 Accessibilty 89
🟢 SEO 100
🟠 PWA 89
Category Score
🟢 First Contentful Paint 0.6 s
🟠 Largest Contentful Paint 3.5 s
🔴 Total Blocking Time 860 ms
🟢 Cumulative Layout Shift 0
🟢 Speed Index 2.4 s

Copy link
Collaborator

@Gilpop8663 Gilpop8663 left a comment

Choose a reason for hiding this comment

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

1. 타입을 범용적으로 바꾸자(그러면 기존 타입이 무용지물이 될 수 있음)
2. 아니다 그냥 지금처럼 PostMenu props를 유지하여 타입의 안정성을 높이자

범용적인 사용을 위해 1번을 택하겠습니다

사용하는 개발자 입장에서 조심하면 된다고 생각해요

그래서 제안하고 싶은 코드는요 현재 있는 PostAction, CommentAction과 같은 타입을 없애고 오로지 MenuItem의 타입만 이용하여서 코드를 작성하는 것을 제안하고 싶어요

export interface MenuItem {
  content: string;
  color: MenuColor;
  action: string;
}

constants.ts

export const POST_ACTION = {
  DELETE: 'delete',
  USER_REPORT: 'userReport',
  POST_REPORT: 'postReport',
  EDIT: 'edit',
} as const;

InnerHeaderPart.tsx

const [action, setAction] = useState<string | null>(null); 

  const handleMenuClick = (action: string) => {
    closeComponent();
    setAction(action);
  };

  {action === POST_ACTION.DELETE && (
          <DeleteModal
            target="POST"
            handleCancelClick={handleCancelClick}
            handleDeleteClick={deletePost}
            isDeleting={isDeletePostLoading}
          />
        )}
        {action === POST_ACTION.POST_REPORT && (
          <ReportModal
            reportType="POST"
            handleReportClick={reportPost}
            handleCancelClick={handleCancelClick}
            isReportLoading={isReportPostLoading}
          />
        )}
        {action === POST_ACTION.USER_REPORT && (
          <ReportModal
            reportType="NICKNAME"
            handleReportClick={reportNickname}
            handleCancelClick={handleCancelClick}
            isReportLoading={isReportNicknameLoading}
          />
        )}

PostMenu.tsx

interface PostMenuProps {
  menuList: MenuItem[];
  handleMenuClick: (menu: string) => void;
}

export default function PostMenu({ menuList, handleMenuClick }: PostMenuProps) {
  return (

여기에 대한 제로와 수아의 의견도 궁금합니다 ~!

fe-리뷰완

@Gilpop8663
Copy link
Collaborator

fe-리뷰완

Copy link
Collaborator

@chsua chsua left a comment

Choose a reason for hiding this comment

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

머리가 아찔합니다 타입 관리 너무 어려워요😂

현재 이름이 postMenu인데 기존 commentMenu/postMenu로 나누어져 있던 것이 하나로 합쳐지기보단 commentMenu가 흡수된 느낌이 들기도 하네요ㅎㅎㅎ

개인적으로는 범용적인 것이 더 좋다고 생각해요.
이전에 존재하던 postMenu를 만들면서 느꼈던 점이 타입 규정을 구체적으로 할수록 재활용이 어려워진다는 점이었어요. 그래서 이 메뉴처럼 common component에 가까운 컴포넌트는 기타 다른 로직과 가능한 분리를 해야 한다고 생각합니다.

저는 다른 것들을 다 차치하고 메뉴 컴포넌트를 과제처럼 누구나 쓰는 common으로 만든다면 어떻게 해야 할까? 부터 생각을 시작을 해보았어요. 그럼 저는 객체 배열로 props를 받을 것 같아요. 아래처럼요

interface MenuItem {
    content: string // 사용자에게 보여질 text
    handleClickEvent: () => {}
}

interface MenuProps {
    menuItemList: MenuItem[]
}

function Menu ({menuItemList}:MenuProps) {
    return (
        <컨테이너> 
            {menuItemList.map((item) => {
                  <button onClick={item.handleClickEvent}>{item.content}<button>
             })}
        </컨테이너>
     )
}

그럼 기존에 있던 color 속성이나 action이 없어집니다.
저는 css 속성을 데이터와 연관을 시키지 않는 것이 좋다고 생각하는 편이라서 color는 그냥 없애고 싶습니다. 데이터와 맵핑하기 보단 css(스타일드 컴포넌트)에서 "삭제"와 같이 강조하고 싶은 부분을 따로 관리하는 것이 더 적절하다고 생각합니다.

action은 기존 로직을 구현하기 위해, 삭제 등을 눌렀을 때 모달이 등장해야 해서 관리했던 걸로 기억합니다. 이는 메뉴 컴포넌트를 만들고 이를 이용하기 보단, 기존 구현한 코드에 기능을 더하기 위해 메뉴를 만든 경우라고 생각됩니다.
전 action이라는 필드가 계속 사용되는데, action 필드가 메뉴 컴포넌트에서 필요한 속성이 아니라고 생각(메뉴를 눌러서 발생하는 거지만 이건 handleClickEvent와 관련이지 메뉴 컴포넌트 자체와는 관련이 없다고 생각)하기 때문에 action이 들어간다면 common이 아니라고 생각이 들어요.
그래서 다시 처음으로 돌아가 어디에서나 사용되는 menu를 만들고 그에 맞춰서 post나 comment 컴포넌트를 수정하는 것이 menu를 common으로 만드는 길이라고 생각합니다. 만약 이 action이 그대로 유지된다면 기존 코드를 모르는 상태에서 common에서 메뉴컴포넌트를 처음보는 사람은 "action: string"인데 뭘 하는 역할인지 의문스러울 것 같아요.

다만 이렇게 했을 때 지금 코드에서 다른 변경사항(타입관련)이 많을 것 같아서 적극적으로 이렇게 하고 싶다고 밀어붙이고 싶지는 않습니다. 제 의견은 이렇습니다.

fe-리뷰완

frontend/src/components/PostMenu/PostMenu.stories.tsx Outdated Show resolved Hide resolved
타입의 범용성 개선, 컴포넌트의 범용성 개선
@inyeong-kang
Copy link
Member Author

inyeong-kang commented Oct 12, 2023

자칭 레전드 리팩터링

우스가 이전에 만든 Select 컴포넌트를 참고하여서, 디자인 시스템에서 Table 컴포넌트를 만들던 도중 제네릭 <T>의 굉장한 유용함을 느낄 수 있었습니다.

PostMenu의 props(menuList)의 타입을 범용적으로 바꾸기 성공!

따라서 제가 pr의 논의사항으로 올린 고민이 무색해져 버렸습니다만.. 아래처럼 PostMenu의 props type을 리팩터링해봤어요!
(또 PostMenu를 common/Menu로 옮겼습니다! 타입이 범용적으로 변경되면서 컴포넌트 자체의 범용성을 높일 수 있었어요😆)

// src/components/Menu
interface MenuProps<T> {
  menuList: MenuItem<T>[];
  handleMenuClick: (menu: T) => void;
}

export default function Menu<T>({ menuList, handleMenuClick }: MenuProps<T>) {
  return (
    <S.Container>
      {menuList.map(({ content, color, action }) => (
        <S.Menu
          key={content}
          type="button"
          tabIndex={0}
          aria-label={content}
          $color={color}
          onClick={(event: MouseEvent) => {
            event.stopPropagation();
            handleMenuClick(action);
          }}
        >
          {content}
        </S.Menu>
      ))}
    </S.Container>
  );
}
  • Menu 컴포넌트 활용 예시
    menuList props로 할당되는 배열의 타입을 적절히 지정해주면 됩니다!!
const NOT_WRITER_POST_MENU_LIST: MenuItem<PostAction>[] = [
  { color: 'black', content: '닉네임 신고', action: 'NICKNAME_REPORT' },
  { color: 'black', content: '게시글 신고', action: 'POST_REPORT' },
];

export const NotPostWriterUser: Story = {
  render: () => <Menu menuList={NOT_WRITER_POST_MENU_LIST} handleMenuClick={() => {}} />,
};

세줄요약 결론

  1. PostMenu props type 유연하게 변경함.
  2. PostMenu 컴포넌트명 Menu로 변경함.
  3. PostAction 과 CommentAction 타입은 그대로 유지하면 됨!

추가로, Menu 컴포넌트는 디자인 시스템으로 분리할 예정입니다~😃

fe-리뷰요청

Copy link
Collaborator

@Gilpop8663 Gilpop8663 left a comment

Choose a reason for hiding this comment

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

저는 컴포넌트에서 T를 받으면 <Menu ... /> 와 같이 사용해야하는 줄 알았는데 자동 추론이 되는군요!

범용적으로 사용이 가능하면서도 자동완성 기능을 지원하고, 안정성을 챙겼다는 점이 인상적이였어요

어프로브하겠습니다 😀😀

@Gilpop8663
Copy link
Collaborator

fe-리뷰완

Copy link
Collaborator

@chsua chsua left a comment

Choose a reason for hiding this comment

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

bb 왕 어떻게 하실지 궁금했는데 를 사용하셨군요!
생각하지 못했는데 너무 좋은 것 같아요.
적절하게 타입이 제한되면서도 사용하는 곳에 따라 적절하게 설정해주면 될 것 같아요!!
최고입니다 제로

fe-리뷰완

interface PostMenuProps {
menuList: PostMenuItem[];
handleMenuClick: (menu: PostAction) => void;
interface MenuProps<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

오 로 받는 군요!!

@chsua chsua merged commit 5b03c9e into dev Oct 12, 2023
1 check passed
@woo-chang woo-chang deleted the refactor/#687 branch October 14, 2023 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[REFACTOR] PostMenu, CommentMenu을 하나의 Menu 컴포넌트로 통합
3 participants