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

홈 화면 서스펜스 범위 재지정 및 셀렉트 컴포넌트 리팩터링 #840

Merged
merged 6 commits into from
Nov 14, 2023

Conversation

Gilpop8663
Copy link
Collaborator

🔥 연관 이슈

close: #839

📝 작업 요약

  • 홈 화면에서 게시글 목록 서스펜스 범위가 게시글 정렬 옵션과 같이 렌더링 되던 것을 게시글 목록만 서스펜스로 감싸주었음
  • 셀렉트 컴포넌트 내부의 isOpen 상태 값을 useSelect 커스텀 훅으로 이동
  • 게시글 목록에서 셀렉트 밖을 누르면 셀렉트가 닫히도록 구현

⏰ 소요 시간

  • 1시간
  • 생각보다 30분이 더 오래 걸렸는데 셀렉트 밖을 누를 때 select에 ref를 넘겨줘야 한다고 생각해서 진행하는데 타입 때문에 오래 걸렸음
  • 셀렉트에 ref를 넘겨주지 않고 셀렉트를 감싸는 SelectWrapper에 ref를 주는 방향으로 생각을 바꿔서 진행

🔎 작업 상세 설명

이전 화면

2023-11-08.5.09.41.mov

바꾼 화면

2023-11-09.10.14.50.mov

셀렉트 닫히는 모습

게시글 목록 컨테이너에 클릭 이벤트를 걸어서 게시글 목록 밖을 클릭했을 때는 닫히지 않아요. 그렇지만 저는 이 정도면 만족해서 PR 올립니다

2023-11-09.10.16.28.mov

Copy link

github-actions bot commented Nov 9, 2023

⚡️ Lighthouse report!

Category Score
🟠 Performance 76
🟢 Accessibilty 91
🟢 SEO 100
🟠 PWA 89
Category Score
🟢 First Contentful Paint 0.6 s
🟠 Largest Contentful Paint 2.9 s
🔴 Total Blocking Time 760 ms
🟢 Cumulative Layout Shift 0.057
🟢 Speed Index 2.4 s

Copy link
Member

@inyeong-kang inyeong-kang left a comment

Choose a reason for hiding this comment

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

우스하면 생각나는 컴포넌트가 Select 컴포넌트인데요,
꾸준한 리팩터링을 통해 재사용성을 높이는 모습이 좋네요👍

컨벤션 관련 사소한 코멘트 남겼습니다😀

const handelOptionChange = (option: SortingOptionType) => {
setSelectedOption(option);
};
export const Disable = () => {
Copy link
Member

Choose a reason for hiding this comment

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

사소한 건데 Disabled 로 해주실 수 있나요 ㅎㅎ

import * as S from './style';

interface PostListFetcherProps {
focusTopContent: () => void;
Copy link
Member

Choose a reason for hiding this comment

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

이벤트 핸들러 함수는 handle-- prefix 붙이는거 어떠신가용

const handleCloseClick = (event: MouseEvent<HTMLDivElement>) => {
if (!selectRef.current) return;

const modalBoundary = selectRef.current.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

오 getBoundingClientRect 는 처음 알았네요! viewport를 기준으로 위치를 계산해서 반환하는 함수군요👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

맞습니다 ㅎㅎ 👍

isDisabled = false,
...rest
}: SelectProps<T>) {
const optionKeyList = Object.keys(optionList) as T[];
const [isOpen, setIsOpen] = useState(false);

const toggleOpen = () => {
Copy link
Member

Choose a reason for hiding this comment

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

handleToggleOpen 어떠신가요😀

@inyeong-kang
Copy link
Member

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

UX가 굉장히 좋아졌습니다!!
이런 셀렉트와 모달 등은 배경을 눌렀을 때 당연히 닫힐 걸로 예상하는데 해당 부분이 안지켜져서 마음이 불편했거든요! 해결되어서 너무 좋아요!
고생하셨어요!

늦게 리뷰해서 죄송해요! fe-리뷰완

toggleSelect: toggleSortingSelect,
selectRef: sortingSelectRef,
handleCloseClick: handleSortingClose,
} = useSelect<PostSorting>(postOption.sorting);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍👍👍

</S.SkeletonWrapper>
}
>
<PostListFetcher handleFocusTopContent={handleFocusTopContent} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍👍 분리 너무 좋아요

@Gilpop8663 Gilpop8663 merged commit 693d046 into dev Nov 14, 2023
1 check passed
@woo-chang woo-chang deleted the feat/#839 branch December 6, 2023 07:07
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.

홈 화면 게시글 목록 서스펜스 범위 재지정
3 participants