-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
⚡️ Lighthouse report!
|
There was a problem hiding this 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-리뷰완
fe-리뷰완 |
There was a problem hiding this 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-리뷰완
타입의 범용성 개선, 컴포넌트의 범용성 개선
자칭 레전드 리팩터링우스가 이전에 만든 Select 컴포넌트를 참고하여서, 디자인 시스템에서 Table 컴포넌트를 만들던 도중 PostMenu의 props(menuList)의 타입을 범용적으로 바꾸기 성공!따라서 제가 pr의 논의사항으로 올린 고민이 무색해져 버렸습니다만.. 아래처럼 PostMenu의 props type을 리팩터링해봤어요! // 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>
);
}
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={() => {}} />,
}; 세줄요약 결론
추가로, Menu 컴포넌트는 디자인 시스템으로 분리할 예정입니다~😃 fe-리뷰요청 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 컴포넌트에서 T를 받으면 <Menu ... /> 와 같이 사용해야하는 줄 알았는데 자동 추론이 되는군요!
범용적으로 사용이 가능하면서도 자동완성 기능을 지원하고, 안정성을 챙겼다는 점이 인상적이였어요
어프로브하겠습니다 😀😀
fe-리뷰완 |
There was a problem hiding this 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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 로 받는 군요!!
🔥 연관 이슈
close: #687
📝 작업 요약
기존의 PostMenu, CommentMenu을 Menu 컴포넌트 하나로 통합하였습니다.
⏰ 소요 시간
2시간
🔎 작업 상세 설명
🌟 논의 사항
여러분의 생각이 궁금합니다! 혹은 더 나은 제안이 있다면 알려주시면 감사하겠습니다~