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

카테고리, 태그, 템플릿 목록 로딩 처리 개선 #784

Open
wants to merge 11 commits into
base: dev/fe
Choose a base branch
from

Conversation

Hain-tain
Copy link
Contributor

@Hain-tain Hain-tain commented Oct 14, 2024

안녕하세요, 프론트 여러분! 헤인입니다.

매번 리팩토링 PR을 올릴 때 files changed가 너무 많아 저도 놀라는데요...
컴포넌트 분리와 훅 분리가 전혀 되지 않은 부분들을 분리하다보니 변경 사항이 많이 잡히게 되는 것 같습니다. 😂
저의 구현 의도와 주요 변경 사항들에 대해 상세하게 써보려고 노력하였으나 부족한 부분이 있다면 얼마든지 코멘트로 질문해주세요!
이름 변경 등 아주 사소한 피드백들도 환영입니다!!


⚡️ 관련 이슈

🪧 주요 목표

  • 카테고리, 태그, 템플릿 목록 로딩 처리를 개선한다.
개선 전 개선 후
태그 하나만 눌러도 모든 페이지 로딩처리 각각 스켈레톤 처리 + 템플릿 목록만 로딩 처리
default.mov
default.mov

📍주요 변경 사항

1. 카테고리, 태그, 템플릿 목록 useSuspenseQueries 적용 해제

  • 카테고리와 태그는 useSuspenseQuery를 사용하여 서스팬스 처리 해주었습니다.
  • 템플릿 목록의 경우 useQuery를 사용하여 isLoding, isFetching 상태로 로딩 처리를 해주었습니다.

2. MyTemplatePage 컴포넌트 분리

  • 페이지 내부에서 사용되는 서브 컴포넌트들을 별도의 파일로 분리하였습니다.
  • 분리된 컴포넌트는 아래와 같습니다.
    • TopBanner: -님의 템플릿 입니다.를 보여주는 컴포넌트
    • CategoryListSection: 카테고리 목록을 불러와 CategoryFilterMenu를 보여주는 컴포넌트
    • CategoryListSectionSkeleton: CategoryListSection이 오기 전에 suspense 에서 보여주는 fallback 컴포넌트
    • TagListSection: 태그 목록을 불러와 TagFilterMenu를 보여주는 컴포넌트
    • TagListSectionSkeleton: TagListSection이 오기 전에 suspense 에서 보여주는 fallback 컴포넌트
    • NewTemplateButton: 템플릿 목록이 없을 경우 템플릿 작성을 유도하는 버튼 컴포넌트
    • TemplateListSection: 템플릿 목록 조회 결과에 따라 NoSearchResults, NewTemplateButton, TemplateGrid 보여주는 컴포넌트
    • TemplateListSectionLoading: 템플릿 목록이 로딩중일 때 로딩을 나타내주는 컴포넌트
    • TemplateDeleteSelection: 템플릿 목록 선택 삭제를 위한 선택 삭제 버튼 및 삭제 확인 모달이 있는 컴포넌트
    • ConfirmDeleteModal: 삭제 확인 모달 컴포넌트

3. useShowTemplateList, useSelectAndDeleteTemplateList 훅으로 로직 분리

  • MyTemplatePage 내부에서 사용되는 상태 및 상태 핸들러 함수들을 역할과 책임에 맞게 분리해보았습니다. ( UI와 로직이 한 파일에서 관리되고 있어, 파일이 몹시 길고 관심사 분리가 되지 않고있다는 느낌을 받았기에 아래와 같이 리팩토링해보았습니다.)
  • 각 훅이 반환하는 값이 너무 많게 느껴지지만 이 이상의 책임 분리가 어렵다고 판단되어 아래와 같이 나눠보았습니다.
  • useShowTemplateList: 선택된 카테고리, 선택된 태그, 키워드, 정렬, 페이지 정보를 기반으로 보여줄 템플릿 목록을 가져오는 로직을 담고 있습니다. 따라서 선택된 카테고리, 선택된 태그, 키워드, 정렬, 페이지 정보를 가지고 있습니다.
  • useSelectAndDeleteTemplateList: 카테고리 목록에서 전체 삭제 및 선택 삭제와 관련된 로직을 담고있습니다. 따라서 현재 편집모드여부, 삭제 모달 열림 여부, 선택된 템플릿 목록 정보를 가지고 있습니다.

🎸기타

  • 현재 router에서 CustomSuspense로 모든 레이아웃을 감싸주고 있어서 Loading 이라고 뜨고 있는데요, 이게 현재 운영 버전(https://www.code-zap.com)에서도 뜨고있습니다. 이게 좋은 사용자 경험은 아니라고 생각되어, CustomSuspense를 잘 꾸미거나 제거하는 방향으로 리팩토링을 해야 할 것 같습니다.

🍗 PR 첫 리뷰 마감 기한

10/16(수) 18:00

@Hain-tain Hain-tain added FE 프론트엔드 refactor 요구사항이 바뀌지 않은 변경사항 labels Oct 14, 2024
@Hain-tain Hain-tain added this to the 6차 스프린트 🦴 milestone Oct 14, 2024
@Hain-tain Hain-tain self-assigned this Oct 14, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

개인적으로 이 훅의 처음 이름은 useTemplateList였는데요, 템플릿 목록을 조회해온다는 느낌을 조금 더 주고 싶어서 고민하다 변경하게 되었습니다. 그러나 여전히 'show'가 들어가는 것이 좋은 이름이라는 생각이 들지않네요🥲

해당 훅의 네이밍 추천받습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

'use' prefix 뒤에 명사가 오는 것이 자연스럽다고 생각되는데, useTemplateListManagement 어떤가요

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 월하랑 비슷한 의견이긴 합니다. form 훅을 useSubmitForm 보다는 useForm으로 많이 쓰는 느낌 아닐까요>? 그런 의미에서 기존 훅 네이밍도 괜찮다고 생각합니다 ㅋㅋㅋ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 의견 감사해요! 고민 끝에 useFilteredTemplateList 라고 바꿔보았습니다. 이 훅의 핵심 역할은 각 조건에 따라 필터링 된 템플릿 목록을 가져오는 것이라고 생각했기 때문입니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오늘 프론트 회의에서 이야기했던 방식대로, 스켈레톤 대신 이전 템플릿 목록 위로 흰색 반투명한 로딩 박스가 뜨도록 했습니다.
그 이유는 스켈레톤보다는 이전 데이터라도 보고있는 것이 사용자가 덜 심심할 것이라고 생각했기 때문입니다!

Comment on lines +98 to +99
{isTemplateListFetching && <TemplateListSectionLoading />}
{!isTemplateListLoading && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(참고) isTemplateListFetchingisTemplateListLoading는 서로 다른 상태입니다.

  • isTemplateListFetching: 새로운 템플릿 목록을 조회해올때마다 true 가 됩니다. 예를 들어 태그를 누르면 그 태그에 해당하는 템플릿 목록을 가져오게 되고, 그동안 true 가 됩니다.
  • isTemplateListLoading: 처음 템플릿 목록을 조회해올 때 true 가 됩니다.

여기서 !isTemplateListLoading를 해주지 않을 경우, 템플릿 목록을 조회하기 이전에 무조건 NewTemplateButton컴포넌트가 보이기 때문에 위처럼 처리해주었습니다.

vi-wolhwa
vi-wolhwa previously approved these changes Oct 16, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

'use' prefix 뒤에 명사가 오는 것이 자연스럽다고 생각되는데, useTemplateListManagement 어떤가요

Comment on lines 3 to 8
export const useKeyword = () => {
const [keyword, handleKeywordChange] = useInput('');
const debouncedKeyword = useDebounce(keyword, 300);

return { keyword, debouncedKeyword, handleKeywordChange };
};
Copy link
Contributor

Choose a reason for hiding this comment

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

useSearchKeyword 같은 네이밍은 어떨까요?

Comment on lines +44 to +47
isEditMode,
toggleIsEditMode,
isDeleteModalOpen,
toggleDeleteModal,
Copy link
Contributor

Choose a reason for hiding this comment

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

edit 모드 변경, deleteModal 오픈 같은 경우에는 해당 훅에서 한번에 묶일 필요가 없지 않을까 합니다.
아마도 handleDelete 때문에 함께 있는 것 같은데, 각각 editMode, deleteModal 훅을 분리하고 handleDelete 함수는 MyTemplatePage에 있어도 되지 않을까 합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

그러면 네이밍도 selectList 관련으로만 지을 수 있지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

개인적으로 선택과 삭제를 하나의 훅에서 관리하게 된 이유는 선택과 삭제가 긴밀하게 연결되어있다고 생각했기 때문입니다. 전체 선택이 가능한 이유는 다른 동작이 가능한 것이 아니라 오로지 삭제를 위한 것이기 때문에 선택은 삭제를 위한 하나의 과정이라고 생각하였습니다.

삭제 모달도 MyTemplate에 있던 것을 TemplateDeleteSelection로 옮긴 것 또한 위와 같은 생각때문이었습니다.

다만, 마위의 의견처럼 선택과 삭제를 유기적인 관점이 아니라 각각 독립된 역할의 관점에서 볼 수도 있을 것 같아요! 이걸 분리하는게 좋을지 그냥 이렇게 합쳐 두는 것이 더 좋을지 고민이 많이 됩니다!! 저의 이런 생각을 들어보시고 고민한 마위와 월하의 생각도 궁금하여 일단 길게 코멘트 남기게 되었습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 월하랑 비슷한 의견이긴 합니다. form 훅을 useSubmitForm 보다는 useForm으로 많이 쓰는 느낌 아닐까요>? 그런 의미에서 기존 훅 네이밍도 괜찮다고 생각합니다 ㅋㅋㅋ

queryKey: [QUERY_KEY.TAG_LIST],
queryFn: () => getTagList({ memberId }),
throwOnError: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 throwOnError가 지워진 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useSuspenseQuery 를 사용하면서 throwOnError 를 사용할 수 없게 되었습니다!
따라서 에러처리를 위해서는 서스펜스 위로 에러바운더리를 따로 넣어주어야 할 것 같습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

const categoryList = categoryData?.categories || [];

return (
<Flex direction='column' gap='2.5rem' style={{ marginTop: '4.5rem' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Flex에 style을 따로 주는 경우가 꽤 있어서 이런 경우에는 따로 styled로 해도 괜찮을 수도 있겠네요,, ㅎㅎ

Comment on lines +7 to +31
const SkeletonButtonWrapper = styled.div`
display: flex;
align-items: center;
justify-content: center;

width: 12.5rem;
height: 3rem; /* CategoryButton과 동일한 높이 */
padding: 0.5rem;

background: linear-gradient(90deg, #e0e0e0 25%, #c0c0c0 50%, #e0e0e0 75%);
background-size: 200% 100%;
border-radius: 8px;
box-shadow: 1px 2px 8px #00000020; /* CategoryButton과 동일한 그림자 */

animation: skeleton-loading 1.5s infinite ease-in-out;

@keyframes skeleton-loading {
0% {
background-position: -200% 0;
}
100% {
background-position: 200% 0;
}
}
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

당장 리팩토링할 필요는 없을거 같지만, 스켈레톤 UI를 width, height만 조정할 수 있게 하면 공통 컴포넌트로 만들 수 있겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 생각이네요! 다만 현재 다른 작업의 우선순위가 더 높다고 생각하기 때문에 이 부분은 일단 인지하고 다음 리펙토링에서 고려해보도록 하겠습니다!

Comment on lines 3 to 7
interface ConfirmDeleteModalProps {
isDeleteModalOpen: boolean;
toggleDeleteModal: () => void;
handleDelete: () => void;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

요기 그냥 Props로 해도 되나요?!


const ConfirmDeleteModal = ({ isDeleteModalOpen, toggleDeleteModal, handleDelete }: ConfirmDeleteModalProps) => (
<Modal isOpen={isDeleteModalOpen} toggleModal={toggleDeleteModal} size='xsmall'>
<Flex direction='column' justify='space-between' align='center' margin='1rem 0 0 0' gap='2rem'>
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 뭔가 style=margin-top과 margin 1 0 0 0 이 비슷해서 위 코멘트를 남겼었습니다 ㅋㅋ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

컴포넌트를 분리할 때 이전 코드를 그대로 복붙해서 사용해서 그런가봅니다. 😂 현재는 Modal 내부 합성 컴포넌트를 활용하여 리펙토링하였습니다!

Copy link
Contributor

@Jaymyong66 Jaymyong66 left a comment

Choose a reason for hiding this comment

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

확인해야할 코멘트가 있어서 RC 남길게요~!
고생하셨습니다 헤인

Copy link
Contributor

@Jaymyong66 Jaymyong66 left a comment

Choose a reason for hiding this comment

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

피드백 반영이 깔끔하게 되었네요! 고생했습니다 헤인
(요즘 헤인 PR 리뷰를 많이 하는 것 같네욥 ㅋㅋ)

Comment on lines +17 to +21
if (result.error && !result.isFetching) {
throw result.error;
}

return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

굿!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 refactor 요구사항이 바뀌지 않은 변경사항
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants