Feat/#333 refactor custom hook #334

ilmerry · 2023-10-01T10:50:03Z

브랜치명, 브랜치 알맞게 설정
Reviewer, Assignees, Label, Milestone, Issue(PR 작성 후에) 붙이기
PR이 승인된 경우 해당 브랜치는 삭제하기
close [ Common ] 공통 커스텀훅 정리하기 #333

📌 내용

useShowByQuery를 삭제하고, useCardType을 사용하도록 고칩니다. 기존에 useShowByQuery를 쓰던 컴포넌트들은 useCardType을 사용하되, isShow~ state를 사용해 보이지 않아야할 Location들에 따라 state를 변경해주고 있습니다.
useDraggableYContainer를 삭제하고, useDraggingContainer가 x축 y축 모든 방향에서 스크롤 가능하도록 변경하였습니다. 이 과정에서, 모바일에는 MouseEvent가 적용되지 않음을 알았습니다. 따라서 handler 함수에서는 Synthetic Event로 모두 받되, getPageByEventType에서 분기처리하여 MouseEvent와 TouchEvent를 구분하고, 기존에는 onMouse~로만 전달되고 있던 props에 onTouch~ 속성을 추가하였습니다.
useRecentlyBookmarked의 위치를 변경하였습니다. 제가 잘못 생각했던 부분이 있는데, "두개 이상의 컴포넌트에서 공통으로 사용한다 = 모두의 공통컴포넌트이다"라고 잘못생각했던 것 같습니다. 앞으로는 이 점 유의하면서 코딩하는게 좋겠습니다!
댓글 창 모달에 사용하는 useDrawer을 리팩토링하였습니다. 로직은 useDraggingContainer와 유사합니다.

📌 내가 알게 된 부분

clientX와 pageX의 차이

디버깅하다가 발견했는데, 현재 상태에서는 clientX와 pageX 모두 같은 값을 찍어내는것을 알았습니다. 그래도 브라우저 창에 의존적인 client보다는, 페이지 내 절대값의 개념인 page를 쓰는것이 안전하겠다고 판단해 page로 통일하겠습니다

joohaem

👍🏼👍🏼👍🏼👍🏼

근데 knob, useDrawer가 뭥가요????(궁금)

joohaem · 2023-10-02T01:04:13Z

src/@components/CardCollectionPage/Card/MenuModal/index.tsx

 export default function MenuModal(props: MenuModalProps) {
  const { currentCardId, closeHandler, autoSlide, handleClickAddBlacklist, handleClickCancelBlacklist } = props;
  const { showToast, blackoutToast } = useToast();

-  const { isShow: isBlockShow } = useShowByCardType([LocationType.BEST, LocationType.MEDLEY]);
+  const { cardType } = useCardType();
+  const [isBlockShow, setIsBlockShow] = useState<boolean>(false);


isBlockShow 뭔가 어법이 이상한 듯한 느낌이...?
-> isBlockShown or couldShowBlock or ... 어떨까 싶어요!

다시보니 그러네요! isBlockVisible은 어떤가요?

joohaem · 2023-10-02T01:06:29Z

src/@components/CardCollectionPage/Card/MenuModal/index.tsx

@@ -23,11 +25,18 @@ type ModalItem = {
  gtmClassName: string;
 };

+const locationTypesNoBlock = [LocationType.BEST, LocationType.MEDLEY];


요런 것도 이 const 배열 객체의 정체성은 'location types' 이므로,
이름이 noBlockLocationTypes가 되는 게 적절해보여요
혹은 locationTypesForNoBlock 처럼 for를 붙여 특수성을 나타내거나?

(+ locationTypesNoReplay 변수명도 동일)

좋은 의견입니다! no를 앞에 붙여 명시하는게 좋을것 같아요

joohaem · 2023-10-02T01:08:23Z

src/@components/@common/hooks/useDraggingContainer.ts

+    const page = dragDirection === "X" ? event.pageX : event.pageY;
+    currentRef.current = page;


👍🏼👍🏼

joohaem · 2023-10-02T01:12:22Z

src/@components/@common/hooks/useDraggingContainer.ts

@@ -1,40 +1,47 @@
 import { useRef, useState } from "react";

-export default function useDraggingContainer() {
+type DragDirectionType = "X" | "Y";


'horizon'도 아닌, 'x'도 아닌, 'X'인 이유가 있을까요?
추상화 레벨 적절하게 열어진 것 다 좋은데, 이 인자값 인터페이스가 조금 어색해보여요
타이핑도 되어있고 해서 지금 상태도 괜찮지만, 가능하다면 좀 더 일반적으로 기대 가능한 인터페이스면 어떨까 싶습니다!!!!!

읽는 사람들(개발자들)에게 x/y는 'x축', 'y축'이라는 통용된 개념으로 읽히기 때문에 horizon/vertical으로 굳이 길게 늘릴 필요가 없다고 생각했어요!

x/y와 X/Y 중 고민했는데, 속성명 중에 pageX, pageY가 있기 때문에 통일성을 주기 위해 X, Y를 사용했습니다.
x/y가 좀 더 가독성이 낫다고 느끼신다면 고쳐보겠습니다!

joohaem · 2023-10-02T01:14:32Z

src/@components/@common/hooks/useDraggingContainer.ts

-  function handleMouseDown(event: React.MouseEvent<HTMLElement, MouseEvent>) {
+  function handleMouseDown(event: React.SyntheticEvent) {


아예 터치 핸들러 따로 만드는 게 좀 더 적절하지 않을까 하는 생각?

joohaem · 2023-10-02T01:23:27Z

src/@components/@common/hooks/useDraggingContainer.ts

-  function handleMouseDown(event: React.MouseEvent<HTMLElement, MouseEvent>) {
+  function handleTriggerDown(event: React.SyntheticEvent<HTMLElement>) {


X, Y를 한 훅에서 처리하는 건 적절해보이는데,
마우스 동작과 터치 동작을 한 함수로 묶는 건 좀 과해보여요
이후에 마우스, 터치 로직들이 추가되고 한다면 함수가 하나의 동작을 못하게 되고 다시 분리하게 되는 현상이 일어날 것 같아서,
차라리 Touch Hanlder 함수를 따로 파주는 게 나을 것 같아요

저도 고민했던 부분인데, MouseDown과 TouchDown 핸들러의 로직이 완전히 동일하기도 해서, 훅 내부가 장황해지는 것을 우려했던 것 같아요. 그래도 아래와 같이 분리하는 게 좋을까요?

function handleMouseDown(event: React.MouseEvent<HTMLElement>) { setIsStartDragging(true); const page = getPageByEventType(event); currentRef.current = page; initializeForDragged(page); } function handleTouchDown(event: React.TouchEvent<HTMLElement>) { setIsStartDragging(true); const page = getPageByEventType(event); currentRef.current = page; initializeForDragged(page); }

저는 분리하는 게 좋아보입니다!
내용이 많으면 내부 코드가 길어지는 건 당연한거지만, 함수간의 목적성이 흐려지는 건 분명한 안티패턴이니까요!

넵 분리하겠습니다! 감사해요:)

joohaem · 2023-10-02T01:24:14Z

src/@components/@common/hooks/useDraggingContainer.ts

+  Y: "pageY",
+};
+
+const FIRST_TOUCH = 0;


more specific naming

터치이벤트가 여러개 발생할 수 있는데, 그 중 첫번째만 사용하고 있으므로 이부분을 인덱스로 따로 뺌
=> FIRST_TOUCH_EVENT_IDX로 변경하겠습니다!

joohaem · 2023-10-02T01:27:03Z

src/@components/@common/hooks/useDrawer.ts

+  const [isStartDragging, setIsStartDragging] = useState<boolean>(false);
+  const [dragged, setDragged] = useState<number>(0);


중복 타이핑

어떤 부분이 중복이죠?!

false, 0으로 초기값 설정하면 타입추론이 될텐데, boolean, number로 한 겹 더 타입을 지정하는 건 되려 휴먼에러를 일으키지 않을까 해서🤔 저는 추론 될 때는 타입 지정을 안 하는 편이에요

오 한번도 생각못해본 거예요. 무작정 타입 지정해주는게 좋다고 생각했었는데! 감사합니다

joohaem · 2023-10-02T01:27:25Z

src/@components/@common/hooks/useDrawer.ts

+  const currentRef = useRef(0);
+  const standardRef = useRef(0);
+
+  const [isStartDragging, setIsStartDragging] = useState<boolean>(false);


isDragging?

joohaem · 2023-10-02T01:27:55Z

src/@components/@common/hooks/useDrawer.ts

+  const standardRef = useRef(0);
+
+  const [isStartDragging, setIsStartDragging] = useState<boolean>(false);
+  const [dragged, setDragged] = useState<number>(0);


more specific naming

draggedDistance로 변경하겠습니다!

NYeonK · 2023-10-02T08:56:38Z

역시 빠르다 은형!!!!🙆‍♂️👏
틈틈이 봐서, 평일 내에 코드리뷰 달아놓을게요! 수고하셨습니다아:)

ilmerry · 2023-10-02T10:13:02Z

👍🏼👍🏼👍🏼👍🏼

근데 knob, useDrawer가 뭥가요????(궁금)

@joohaem useDrawer는 댓글창 모달에 사용하는 훅이에요. 이미지에 보다시피 drawer는 상단의 손잡이(knob)를 가지고 컨트롤하는 모달입니다! 따라서 모달 자체에 해당하는 ref도 필요하고, 손잡이에 해당하는 ref도 필요합니다🙂

ilmerry added 13 commits September 26, 2023 20:40

[ refactor ] useShowByQuery to useCardType

05c5b0d

[ etc ] remove useShowByQuery

300f4cb

[ etc ] move useRecentlyBookmarked and useCardsByGender

2beb678

[ refactor ] useDraggableYContainer to useDraggingContainer

fb6e963

[ fix ] mobile undragging error

903cc2d

[ refactor ] eventHandler using SyntheicEvent

6a60f14

[ fix ] unDragging problem caused by gradient

8cce380

[ refactor ] rename handlers

4b16682

[ refactor ] add event mapper

5ff0135

[ refactor ] add getPageByEventType

c1b1a1c

[ fix ] active gradient

a5e073f

[ etc ] remove useDraggableYContainer

a2e5c18

[ refactor ] useDrawer

6c7b179

ilmerry added Refactor 은형 labels Oct 1, 2023

ilmerry self-assigned this Oct 1, 2023

ilmerry requested review from NYeonK and joohaem October 1, 2023 10:50

ilmerry linked an issue Oct 1, 2023 that may be closed by this pull request

[ Common ] 공통 커스텀훅 정리하기 #333

Open

4 tasks

[ refactor ] page, client 통일

15bf514

joohaem reviewed Oct 2, 2023

View reviewed changes

ilmerry added 3 commits October 6, 2023 09:54

[ refactor ] 중복타이핑 제거 및 변수명 변경

54bf17c

[ refactor ] MouseHandler와 TouchHandler 분리하기

902fc59

[ refactor ] handler 함수명 재변경

16547fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/#333 refactor custom hook #334

Feat/#333 refactor custom hook #334

ilmerry commented Oct 1, 2023 •

edited

Loading

joohaem left a comment

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

joohaem Oct 2, 2023

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

joohaem Oct 2, 2023

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

joohaem Oct 6, 2023 •

edited

Loading

ilmerry Oct 6, 2023

joohaem Oct 2, 2023

ilmerry Oct 2, 2023 •

edited

Loading

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

joohaem Oct 6, 2023

ilmerry Oct 6, 2023

joohaem Oct 2, 2023

joohaem Oct 2, 2023

ilmerry Oct 2, 2023

NYeonK commented Oct 2, 2023

ilmerry commented Oct 2, 2023

		const page = dragDirection === "X" ? event.pageX : event.pageY;
		currentRef.current = page;

		function handleMouseDown(event: React.MouseEvent<HTMLElement, MouseEvent>) {
		function handleMouseDown(event: React.SyntheticEvent) {

		function handleMouseDown(event: React.MouseEvent<HTMLElement, MouseEvent>) {
		function handleTriggerDown(event: React.SyntheticEvent<HTMLElement>) {

		const [isStartDragging, setIsStartDragging] = useState<boolean>(false);
		const [dragged, setDragged] = useState<number>(0);

Feat/#333 refactor custom hook #334

Are you sure you want to change the base?

Feat/#333 refactor custom hook #334

Conversation

ilmerry commented Oct 1, 2023 • edited Loading

📌 내용

📌 내가 알게 된 부분

joohaem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joohaem Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmerry Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NYeonK commented Oct 2, 2023

ilmerry commented Oct 2, 2023

ilmerry commented Oct 1, 2023 •

edited

Loading

joohaem Oct 6, 2023 •

edited

Loading

ilmerry Oct 2, 2023 •

edited

Loading